-
Notifications
You must be signed in to change notification settings - Fork 760
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: document public APIs in event
module
#2426
Conversation
This change adds documentation to the event module and all the public APIs within it. This includes doctests on all the methods which serve as examples. Some small API changes are also included The methods `with_explicit_parent` and `with_contextual_parent` have had the negative case moved into separate methods `without_explicit_parent` and `without_contextual_parent` respectively. This removes the need for an `Option` around the parameter in the original methods - which are likely to be the most used case (the 'without' case where `None` would have been passed before is not used anywhere in this workspace). The method `in_scope` has been placed behind the `tracing-subscriber` feature flag as it currently only works with the `MockSubscriber`, not with the `MockCollector`. If the feature flag is active and it is used to set a non-empty scope, the `MockCollector` will panic with `unimplemented` during validation. Refs: #539
a073f03
to
eacabb5
Compare
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.
Thanks!
I had some suggestions for improving the documentation here a little bit, let me know what you think!
Also, if you're willing to put a bit more time into this, it would be really nice if the docs had examples showing a case where an expectation should pass, and also showing a case where the expectation should fail. If that's too much extra work, though, don't worry about it.
These are mostly the simple changes and exclude the with/without contextual/explicit parent topic which needs a bit more work. Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Thank you so much for all the suggestions! Most of them I've applied directly. For the topic around the contextual/explicit parents I'm going to prepare a separate commit, I also need to understand a bit the situation around a contextual parent, because your description doesn't match how I thought some of it worked! I'm also more than happy to add in the negative tests. I already had it on my list of things to do, but had thought to put them into a separate test file (as opposed to in the examples) as I was worried that the API docs were getting too long. However I think you're right that they'd be better in the API docs as for a crate like |
There is additional work needed to clean up `with_explicit_parent` and `with_contextual_parent`, so they've been reverted back to how they are in master. The comments from the PR review have been applied and both functions have examples for expecting a parent span, expecting that the event is a root, and a negative example of a failing expectation.
@hawkw I believe I've applied all the suggestions. Please let me know if there is anything further, I don't mind going through further review cycles with this, as many as we need. |
There were copy-paste errors in the `with_explicit_parent` and `with_contextual_parent` descriptions. The description of `in_scope` was also improved and the note about it only working with `MockSubscriber` was corrected to state that if used with `MockCollector` it will fail directly (as opposed to not doing anything).
…o hds/mock-docs-event
@hds thanks for the ping, I'll take a look now! |
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.
I suggested some very minor edits to the documentation. Once those are addressed, I'll be happy to merge this!
Thanks Eliza! Co-authored-by: Eliza Weisman <eliza@buoyant.io>
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.
Changes applied. Thanks!
Also cleaned up the text and tried to apply the code review comments from #2426 to what I'd written already.
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 looks great, besides one last typo (sorry!). I'm gonna go ahead and merge it now!
Thank you for fixing that last typo! And thanks for all the very detailed reviews! |
There has been interest around publishing tracing-mock to crates.io for some time. In order to make this possible, documentation and some code clean up is needed. The `event` module needs documentation and examples. This change adds documentation to the event module and all the public APIs within it. This includes doctests on all the methods which serve as examples. The following pattern was applied to the description of most methods: - Short description of expectation - Additional clarification (where needed) - Description of cases that cause the expectation to fail - Examples - Successful validation - Unsuccesful validation Two changes were also made in the text provided to the user when an assertion fails for `with_explicit_parent` or `with_contextual_parent`. One small API changes is also included: The method `in_scope` has been placed behind the `tracing-subscriber` feature flag as it currently only works with the `MockSubscriber`, not with the `MockCollector`. If the feature flag is active and it is used to set a non-empty scope, the `MockCollector` will panic with `unimplemented` during validation. Refs: #539
There has been interest around publishing tracing-mock to crates.io for some time. In order to make this possible, documentation and some code clean up is needed. The `event` module needs documentation and examples. This change adds documentation to the event module and all the public APIs within it. This includes doctests on all the methods which serve as examples. The following pattern was applied to the description of most methods: - Short description of expectation - Additional clarification (where needed) - Description of cases that cause the expectation to fail - Examples - Successful validation - Unsuccesful validation Two changes were also made in the text provided to the user when an assertion fails for `with_explicit_parent` or `with_contextual_parent`. One small API changes is also included: The method `in_scope` has been placed behind the `tracing-subscriber` feature flag as it currently only works with the `MockSubscriber`, not with the `MockCollector`. If the feature flag is active and it is used to set a non-empty scope, the `MockCollector` will panic with `unimplemented` during validation. Refs: #539
There has been interest around publishing tracing-mock to crates.io for some time. In order to make this possible, documentation and some code clean up is needed. The `event` module needs documentation and examples. This change adds documentation to the event module and all the public APIs within it. This includes doctests on all the methods which serve as examples. The following pattern was applied to the description of most methods: - Short description of expectation - Additional clarification (where needed) - Description of cases that cause the expectation to fail - Examples - Successful validation - Unsuccesful validation Two changes were also made in the text provided to the user when an assertion fails for `with_explicit_parent` or `with_contextual_parent`. One small API changes is also included: The method `in_scope` has been placed behind the `tracing-subscriber` feature flag as it currently only works with the `MockSubscriber`, not with the `MockCollector`. If the feature flag is active and it is used to set a non-empty scope, the `MockCollector` will panic with `unimplemented` during validation. Refs: tokio-rs#539
Motivation
There has been interest around publishing tracing-mock to crates.io
for some time. In order to make this possible, documentation and some
code clean up is needed.
The
event
module needs documentation and examples.Solution
This change adds documentation to the event module and all the public
APIs within it. This includes doctests on all the methods which serve as
examples.
The following pattern was applied to the description of most methods:
Two changes were also made in the text provided to the user when an
assertion fails for
with_explicit_parent
orwith_contextual_parent
.One small API changes is also included:
The method
in_scope
has been placed behind thetracing-subscriber
feature flag as it currently only works with the
MockSubscriber
, notwith the
MockCollector
. If the feature flag is active and it is usedto set a non-empty scope, the
MockCollector
will panic withunimplemented
during validation.Refs: #539