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

Add deny(broken_intra_doc_links) #981

Merged
merged 16 commits into from Sep 23, 2020
Merged

Conversation

TaKO8Ki
Copy link
Contributor

@TaKO8Ki TaKO8Ki commented Sep 21, 2020

Motivation

In order to get a compiler warning (or error) when links are broken.
closes #940

Solution

  • add deny(broken_intra_doc_links)
  • add a note to the CONTRIBUTING.md documentation on building docs locally

@TaKO8Ki TaKO8Ki requested review from hawkw, yaahc and a team as code owners September 21, 2020 06:48
@TaKO8Ki TaKO8Ki marked this pull request as draft September 21, 2020 06:48
@TaKO8Ki
Copy link
Contributor Author

TaKO8Ki commented Sep 21, 2020

Should I add deny(broken_intra_doc_links) all tracing crates?

@TaKO8Ki TaKO8Ki changed the title [WIP] Add deny(broken_intra_doc_links) Add deny(broken_intra_doc_links) Sep 21, 2020
@TaKO8Ki TaKO8Ki marked this pull request as ready for review September 21, 2020 09:23
@TaKO8Ki
Copy link
Contributor Author

TaKO8Ki commented Sep 21, 2020

Which section of CONTRIBUTING.md should I add a note of the way to build docs locally?

@jyn514
Copy link
Contributor

jyn514 commented Sep 21, 2020

Should I add deny(broken_intra_doc_links) all tracing crates?

You don't need this to enable intra-doc links, only to make them a hard error. They'll already warn by default if they fail to resolve.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me overall. i had a couple style nits that i'd prefer to see addressed.

as a follow-up, we should start converting existing URL-based links into intra-doc links whenever possible.

Which section of CONTRIBUTING.md should I add a note of the way to build docs locally?

I would probably add a new subsection under the "Pull Requests" section, maybe titled "Building Documentation" or something similar. I'd probably put this after the "Tests" subsection, so right before the "Commits" subsection.

tracing-core/src/callsite.rs Outdated Show resolved Hide resolved
@@ -91,7 +91,7 @@
issue_tracker_base_url = "https://github.com/tokio-rs/tracing/issues/"
)]
#![cfg_attr(not(feature = "std"), no_std)]
#![cfg_attr(docsrs, feature(doc_cfg))]
#![cfg_attr(docsrs, feature(doc_cfg), deny(broken_intra_doc_links))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, i've never seen this form of cfg_attr with multiple attributes before, does this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even know about it until a while ago, but rust-lang/rust#54862 Implemented cfg_attr with multiple attributes!

@@ -114,7 +114,7 @@ pub struct Identifier(
/// [`enabled`] is evaluated for every event.
///
/// This function will also re-compute the global maximum level as determined by
/// the [`Subscriber::max_level_hint`] method. If a [`Subscriber`]
/// the [`Subscriber::max_level_hint`](crate::Subscriber::max_level_hint) method. If a [`Subscriber`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elsewhere, we tend to put link references at the end of the doc comment, rather than inline. i have a slight preference for doing this consistently with the newly-added link references, but i'm not going to block merging this on that.

also, we've tried to wrap the doc comments at a consistent column, can we continue doing that with the changes here?

Copy link
Contributor Author

@TaKO8Ki TaKO8Ki Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can!

Copy link
Contributor Author

@TaKO8Ki TaKO8Ki Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we tend to put link references at the end of the doc comment, rather than inline

I've fixed this!

Copy link
Contributor Author

@TaKO8Ki TaKO8Ki Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've tried to wrap the doc comments at a consistent column, can we continue doing that with the changes here?

What exactly do I have to do?

@TaKO8Ki TaKO8Ki requested a review from hawkw September 21, 2020 18:18
CONTRIBUTING.md Outdated
@@ -211,6 +211,14 @@ To reduce the effort required to review documentation-related changes,
be viewed by clicking the `details` link on the
`netlify/tracing-rs/deploy-preview check` on all pull requests.

### Building Documentation

If you want to build docs locally, you should do like this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to explain briefly why this is necessary rather than just running cargo doc

Comment on lines 638 to 639
//! This crate provides two feature flags, "log" and "log-always", which will
//! cause [spans] and [events] to emit `log` records. When the "log" feature is
//! cause [spans][span] and [events][event] to emit `log` records. When the "log" feature is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wrap this at the same column as the other lines in this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this!

tracing/src/lib.rs Outdated Show resolved Hide resolved
tracing/src/lib.rs Outdated Show resolved Hide resolved
@TaKO8Ki TaKO8Ki requested a review from hawkw September 22, 2020 05:15
Copy link
Contributor

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, last comment I promise 😆

tracing/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last suggestion for the CONTRIBUTING.md docs, and then I think this is good to merge!

CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@TaKO8Ki TaKO8Ki requested a review from hawkw September 23, 2020 02:44
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! thanks for working on this!

@hawkw hawkw merged commit 6f01226 into tokio-rs:master Sep 23, 2020
hawkw added a commit that referenced this pull request Sep 28, 2020
Fixed

- Incorrect inlining of `Span::new`, `Span::new_root`, and
  `Span::new_child_of`, which could result in `dispatcher::get_default`
  being  inlined at the callsite ([#994])
- Regression where using a struct field as a span or event field when
  other fields on that struct are borrowed mutably would fail to compile
  ([#987])

Changed

- Updated `tracing-core` to 0.1.17 ([#992])

Added

- `Instrument` trait and `Instrumented` type for attaching a `Span` to a
  `Future` ([#808])
- `Copy` implementations for `Level` and `LevelFilter` ([#992])
- Multiple documentation fixes and improvements ([#964], [#980], [#981])

Thanks to @nagisa, and new contributors @securityinsanity, @froydnj,
@jyn514 and @TaKO8Ki for contributing to this release!

[#994]: #994
[#992]: #992
[#987]: #987
[#980]: #980
[#981]: #981
[#964]: #964
[#808]: #808

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Sep 28, 2020
### Fixed

- Incorrect inlining of `Span::new`, `Span::new_root`, and
  `Span::new_child_of`, which could result in `dispatcher::get_default`
  being  inlined at the callsite ([#994])
- Regression where using a struct field as a span or event field when
  other fields on that struct are borrowed mutably would fail to compile
  ([#987])

### Changed

- Updated `tracing-core` to 0.1.17 ([#992])

### Added

- `Instrument` trait and `Instrumented` type for attaching a `Span` to a
  `Future` ([#808])
- `Copy` implementations for `Level` and `LevelFilter` ([#992])
- Multiple documentation fixes and improvements ([#964], [#980], [#981])

Thanks to @nagisa, and new contributors @securityinsanity, @froydnj,
@jyn514 and @TaKO8Ki for contributing to this release!

[#994]: #994
[#992]: #992
[#987]: #987
[#980]: #980
[#981]: #981
[#964]: #964
[#808]: #808

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Feb 4, 2021
Fixed

- Compiler error when using `#[instrument(err)]` on functions with
  mutable parameters ([#1167])
- Missing function visibility modifier when using `#[instrument]` with
  `async-trait` ([#977])
- Multiple documentation fixes and improvements ([#965], [#981],
  [#1215])

 Changed

- `tracing-futures` dependency is no longer required when using
  `#[instrument]` on async functions ([#808])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to
this release!

[#1167]: #1167
[#977]: #977
[#965]: #965
[#981]: #981
[#1215]: #1215
[#808]: #808
hawkw added a commit that referenced this pull request Feb 4, 2021
Fixed

- Compiler error when using `#[instrument(err)]` on functions with
  mutable parameters ([#1167])
- Missing function visibility modifier when using `#[instrument]` with
  `async-trait` ([#977])
- Multiple documentation fixes and improvements ([#965], [#981],
  [#1215])

 Changed

- `tracing-futures` dependency is no longer required when using
  `#[instrument]` on async functions ([#808])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to
this release!

[#1167]: #1167
[#977]: #977
[#965]: #965
[#981]: #981
[#1215]: #1215
[#808]: #808
hawkw added a commit that referenced this pull request Feb 4, 2021
### Fixed

- Compiler error when using `#[instrument(err)]` on functions with
  mutable parameters ([#1167])
- Missing function visibility modifier when using `#[instrument]` with
  `async-trait` ([#977])
- Multiple documentation fixes and improvements ([#965], [#981],
  [#1215])

 ### Changed

- `tracing-futures` dependency is no longer required when using
  `#[instrument]` on async functions ([#808])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to
this release!

[#1167]: #1167
[#977]: #977
[#965]: #965
[#981]: #981
[#1215]: #1215
[#808]: #808
hawkw added a commit that referenced this pull request Feb 4, 2021
Fixed

- **attributes**: Compiler error when using `#[instrument(err)]` on
  functions with mutable parameters ([#1167])
- **attributes**: Missing function visibility modifier when using
  `#[instrument]` with `async-trait` ([#977])
- **attributes** Removed unused `syn` features ([#928])
- **log**: Fixed an issue where the `tracing` macros would generate code
  for events whose levels are disabled statically by the `log` crate's
  `static_max_level_XXX` features ([#1175])
- Fixed deprecations and clippy lints ([#1195])
- Several documentation fixes and improvements ([#941], [#965], [#981],
  [#1146], [#1215])

Changed

- **attributes**: `tracing-futures` dependency is no longer required
  when using `#[instrument]` on async functions ([#808])
- **attributes**: Updated `tracing-attributes` minimum dependency to
  v0.1.12 ([#1222])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for
contributing to this release!
hawkw added a commit that referenced this pull request Feb 4, 2021
### Fixed

- **attributes**: Compiler error when using `#[instrument(err)]` on
  functions with mutable parameters ([#1167])
- **attributes**: Missing function visibility modifier when using
  `#[instrument]` with `async-trait` ([#977])
- **attributes** Removed unused `syn` features ([#928])
- **log**: Fixed an issue where the `tracing` macros would generate code
  for events whose levels are disabled statically by the `log` crate's
  `static_max_level_XXX` features ([#1175])
- Fixed deprecations and clippy lints ([#1195])
- Several documentation fixes and improvements ([#941], [#965], [#981],
  [#1146], [#1215])

### Changed

- **attributes**: `tracing-futures` dependency is no longer required
  when using `#[instrument]` on async functions ([#808])
- **attributes**: Updated `tracing-attributes` minimum dependency to
  v0.1.12 ([#1222])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for
contributing to this release!
hawkw added a commit that referenced this pull request Feb 16, 2021
hawkw added a commit that referenced this pull request Feb 19, 2021
Added

- Re-export the `log` crate so that users can ensure consistent versions
  ([#602])
- `AsLog` implementation for `tracing::LevelFilter` ([#1248])
- `AsTrace` implementation for `log::LevelFilter` ([#1248])

Fixed

- **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer`
  not calling `Subscriber::enabled` ([#1254])
- **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer`
  not checking the max level hint ([#1247])
- Several documentation fixes ([#483], [#485], [#537], [#595], [#941],
  [#981])

[#483]: https://github.com/tokio-rs/tracing/pulls/483
[#485]: https://github.com/tokio-rs/tracing/pulls/485
[#537]: https://github.com/tokio-rs/tracing/pulls/537
[#595]: https://github.com/tokio-rs/tracing/pulls/595
[#605]: https://github.com/tokio-rs/tracing/pulls/604
[#941]: https://github.com/tokio-rs/tracing/pulls/941
[#1247]: https://github.com/tokio-rs/tracing/pulls/1247
[#1248]: https://github.com/tokio-rs/tracing/pulls/1248
[#1254]: https://github.com/tokio-rs/tracing/pulls/1254

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

docs: use intra-RustDoc links rather than URLs
3 participants