-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mock: change helper functions to expect::<thing>
#2377
Conversation
The current format of test expectations in `tracing-mock` isn't ideal. The format `span::expect` requires importing `tracing_mock::<thing>` which may conflict with imports from other tracing crates, especially `tracing-core`. So we change the order and move the functions into a module called `expect` so that: * `event::expect` becomes `expect::event` * `span::expect` becomes `expect::span` * `field::expect` becomes `expect::field` This format has two advantages. 1. It reads as natural English, e.g "expect span" 2. It is no longer common to import the modules directly. Regarding point (2), the following format was previously common: ```rust use tracing_mock::field; field::expect(); ``` This import of the `field` module may then conflict with importing the same from `tracing_core`, making it necessary to rename one of the imports. The same code would now be written: ```rust use tracing_mock::expect; expect::field(); ``` Which is less likely to conflict. This change also fixes an unused warning on `MockHandle::new` when the `tracing-subscriber` feature is not enabled. Refs: #539
50781e8
to
4de00e0
Compare
looks like rustfmt needs to be run? https://github.com/tokio-rs/tracing/actions/runs/3446620438/jobs/5751783258 |
Whoops. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new API looks great, thanks!
there's probably some places where imports in our existing tests can now be made a bit nicer, since we can import the tracing::{span, event}
modules without clashing, but i think it's fine to not do that in this branch, especially since the diff is already quite large.
pub fn event() -> ExpectedEvent { | ||
ExpectedEvent { | ||
..Default::default() | ||
} | ||
} | ||
|
||
pub fn field<K>(name: K) -> ExpectedField | ||
where | ||
String: From<K>, | ||
{ | ||
ExpectedField { | ||
name: name.into(), | ||
value: ExpectedValue::Any, | ||
} | ||
} | ||
|
||
pub fn span() -> ExpectedSpan { | ||
ExpectedSpan { | ||
..Default::default() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unrelated to this PR, but it occurs to me that these probably ought to have #[must_use]
annotations on them, since calling these functions and dropping the result will do nothing. we should probably do a pass to add #[must_use]
annotations as part of the release prep, in a follow-up PR...
* mock: change helper functions to `expect::<thing>` The current format of test expectations in `tracing-mock` isn't ideal. The format `span::expect` requires importing `tracing_mock::<thing>` which may conflict with imports from other tracing crates, especially `tracing-core`. So we change the order and move the functions into a module called `expect` so that: * `event::expect` becomes `expect::event` * `span::expect` becomes `expect::span` * `field::expect` becomes `expect::field` This format has two advantages. 1. It reads as natural English, e.g "expect span" 2. It is no longer common to import the modules directly. Regarding point (2), the following format was previously common: ```rust use tracing_mock::field; field::expect(); ``` This import of the `field` module may then conflict with importing the same from `tracing_core`, making it necessary to rename one of the imports. The same code would now be written: ```rust use tracing_mock::expect; expect::field(); ``` Which is less likely to conflict. This change also fixes an unused warning on `MockHandle::new` when the `tracing-subscriber` feature is not enabled. Refs: #539
* mock: change helper functions to `expect::<thing>` The current format of test expectations in `tracing-mock` isn't ideal. The format `span::expect` requires importing `tracing_mock::<thing>` which may conflict with imports from other tracing crates, especially `tracing-core`. So we change the order and move the functions into a module called `expect` so that: * `event::expect` becomes `expect::event` * `span::expect` becomes `expect::span` * `field::expect` becomes `expect::field` This format has two advantages. 1. It reads as natural English, e.g "expect span" 2. It is no longer common to import the modules directly. Regarding point (2), the following format was previously common: ```rust use tracing_mock::field; field::expect(); ``` This import of the `field` module may then conflict with importing the same from `tracing_core`, making it necessary to rename one of the imports. The same code would now be written: ```rust use tracing_mock::expect; expect::field(); ``` Which is less likely to conflict. This change also fixes an unused warning on `MockHandle::new` when the `tracing-subscriber` feature is not enabled. Refs: #539
* mock: change helper functions to `expect::<thing>` The current format of test expectations in `tracing-mock` isn't ideal. The format `span::expect` requires importing `tracing_mock::<thing>` which may conflict with imports from other tracing crates, especially `tracing-core`. So we change the order and move the functions into a module called `expect` so that: * `event::expect` becomes `expect::event` * `span::expect` becomes `expect::span` * `field::expect` becomes `expect::field` This format has two advantages. 1. It reads as natural English, e.g "expect span" 2. It is no longer common to import the modules directly. Regarding point (2), the following format was previously common: ```rust use tracing_mock::field; field::expect(); ``` This import of the `field` module may then conflict with importing the same from `tracing_core`, making it necessary to rename one of the imports. The same code would now be written: ```rust use tracing_mock::expect; expect::field(); ``` Which is less likely to conflict. This change also fixes an unused warning on `MockHandle::new` when the `tracing-subscriber` feature is not enabled. Refs: tokio-rs#539
Motivation
The current format of test expectations in
tracing-mock
isn't ideal. The formatspan::expect
requires importingtracing_mock::<thing>
which may conflict with imports from other tracing crates, especiallytracing-core
.Solution
So we change the order and move the functions into a module called
expect
so that:event::expect
becomesexpect::event
span::expect
becomesexpect::span
field::expect
becomesexpect::field
This format has two advantages.
Regarding point (2), the following format was previously common:
This import of the
field
module may then conflict with importing the same fromtracing_core
, making it necessary to rename one of the imports.The same code would now be written:
Which is less likely to conflict.
This change also fixes an unused warning on
MockHandle::new
when thetracing-subscriber
feature is not enabled.Refs: #539