Skip to content
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

docs: enable and clean up ignored doc tests #2893

Closed
wants to merge 2 commits into from

Conversation

mladedav
Copy link
Contributor

Motivation

Some tests were ignored with a TODO to enable them.

The doc-test for Subscriber::record provides misleading code that does not compile.

Solution

I enabled the tests that can be enabled.

I fixed the usage of omitted fields in Subscriber::record docs, but since the example uses a macro from tracing, it cannot be enabled.

@mladedav mladedav requested review from hawkw and a team as code owners February 26, 2024 09:24
@kaffarell
Copy link
Contributor

I think we can also add a few other ones (these all seem to work for me):

  • tracing/src/span.rs:665
  • tracing/src/span.rs:689
  • tracing/src/span.rs:706
  • tracing/src/lib.rs:158

@hawkw
Copy link
Member

hawkw commented Mar 1, 2024

Hi @mladedav , thanks for the fix! Would you mind checking whether these tests are also ignored on themaster branch, and if so, changing this PR to be based on the master branch? We prefer to merge changes that apply to both the v0.1.x and master branches to master, and then backport them to v0.1.x. This helps ensure that bugs fixed in the release version are also fixed in v0.2. Thank you!

If this only applies to v0.1.x, then I'll be happy to merge it as-is.

@mladedav
Copy link
Contributor Author

mladedav commented Mar 1, 2024

@hawkw sure, I opened #2896 for master. I wasn't sure and guessed (wrong) that this branch is the one to do this since this would be most likely the the one from which docs are generated.

There are just two conflicts, one example in subscriber seems to have been simplified and enabled so it didn't need the changes here and a some code seems to have disappeared from the datetime formatting but I didn't dig deeper. Feel free to reuse this or cherry pick the one on master and close this.

Thanks @kaffarell for the check, I greped just for ```rust,ignore, I forgot the rust part is optional. Now this hopefully covers everything.

@mladedav mladedav force-pushed the dm/ignored-doc-test branch from 6d03e42 to 7afd6b7 Compare March 2, 2024 21:50
@mladedav
Copy link
Contributor Author

Superseded by #2896

@mladedav mladedav closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants