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: improved the linking inside and outside of the crate #618

Merged
merged 12 commits into from
Sep 27, 2024

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Sep 26, 2024

While browsing the docs on docs.rs, I noticed that there are a few items which are not linked.
This PR tries to improve this situation.

Drawing attention to one thing which might be controversial:
In the current codebase, the usage of [´foo´](Self::foo) and [´Self::foo´] (and related links) are a bit inconsistent. I have changed all to the [´Self::foo´] format. Please let me know if you prefer the first and I can change them to that format.

I have verified this change via adding the following (somewhat agressive) docs-related lints and running RUSTDOCFLAGS="--cfg=docsrs" cargo +nightly doc --no-deps --all-features and cargo clippy --all-features --all.

#![warn(clippy::doc_markdown)]
#![warn(rustdoc::all)]

/// will be serialized in YAML format. This does mean that unlike the debug
/// snapshot variant the type of the value does not appear in the output.
/// You can however use the `assert_ron_snapshot!` macro to dump out
/// You can however use the [`crate::assert_ron_snapshot!`] macro to dump out
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these crate specs are too much structure iff the links work without them. Fine if they're required on balance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jea.. I have no clue why they are required..
(the other references are equivalently nessesary sadly.. maybe marcros and scoping? Would need to debug further, but currently don't have time for diving into rustdoc^^)

This seems more like a bug, right?

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the feature enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, have just double checked myself. I did activate all features:

cargo doc --all-features --all --keep-going 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also reproducible via make doc, aka

RUSTC_BOOTSTRAP=1 RUSTDOCFLAGS="--cfg=docsrs" cargo doc --no-deps --all-features

Copy link
Collaborator

@max-sixty max-sixty Sep 27, 2024

Choose a reason for hiding this comment

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

Weirdly I can't repro this...

edit: I repro with cargo doc (as well as others), but not cargo doc --all-features...

Copy link
Contributor Author

@CommanderStorm CommanderStorm Sep 27, 2024

Choose a reason for hiding this comment

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

Here are details to reproduce :

rustup --version
rustup 1.27.1 (54dd3d00f 2024-04-24)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.81.0 (eeb90cda1 2024-09-04)`
git diff | cat
diff --git a/insta/src/macros.rs b/insta/src/macros.rs
index 45434b9..9c69005 100644
--- a/insta/src/macros.rs
+++ b/insta/src/macros.rs
@@ -81,7 +81,7 @@ macro_rules! assert_toml_snapshot {
 /// The value needs to implement the [`serde::Serialize`] trait and the snapshot
 /// will be serialized in YAML format.  This does mean that unlike the debug
 /// snapshot variant the type of the value does not appear in the output.
-/// You can however use the [`assert_ron_snapshot!`](crate::assert_ron_snapshot!) macro to dump out
+/// You can however use the [`assert_ron_snapshot!`] macro to dump out
 /// the value in [RON](https://github.com/ron-rs/ron/) format which retains some
 /// type information for more accurate comparisons.
 ///
RUSTC_BOOTSTRAP=1 RUSTDOCFLAGS="--cfg=docsrs" cargo doc --no-deps --all-features --keep-going
    Checking insta v1.41.0 (/tmp/insta/insta)
 Documenting insta v1.41.0 (/tmp/insta/insta)
 Documenting cargo-insta v1.41.0 (/tmp/insta/cargo-insta)
warning: unresolved link to `assert_ron_snapshot`
   --> insta/src/macros.rs:77:1
    |
77  | / /// Asserts a [`serde::Serialize`] snapshot in YAML format.
78  | | ///
79  | | /// **Feature:** `yaml`
80  | | ///
...   |
120 | | ///
121 | | /// The snapshot name is optional but can be provided as first argument.
    | |________________________________________________________________________^
    |
    = note: the link appears in this line:
            
            You can however use the [`assert_ron_snapshot!`] macro to dump out
                                     ^^^^^^^^^^^^^^^^^^^^^^
    = note: no item named `assert_ron_snapshot` in scope
    = note: `macro_rules` named `assert_ron_snapshot` exists in this crate, but it is not in scope at this link's location
note: the lint level is defined here
   --> insta/src/lib.rs:2:9
    |
2   | #![warn(rustdoc::all)]
    |         ^^^^^^^^^^^^
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` implied by `#[warn(rustdoc::all)]`

warning: `insta` (lib doc) generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.19s
   Generated /tmp/insta/target/doc/cargo_insta/index.html and 1 other file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, sorry, I hadn't realized this was with changed code

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Overall great, but I think we should scale it back about 30% — anything that doesn't require crate:: or self:: or std::fmt, etc could be lighter. Is that reasonable?

Happy to try enabling some of the lints if they are manageable to work with... We can ignore the vendored path though; that code won't see much development.

@CommanderStorm
Copy link
Contributor Author

Drawing attention to one thing which might be controversial:
In the current codebase, the usage of [´foo´](Self::foo) and [´Self::foo´] (and related links) are a bit inconsistent. I have changed all to the [´Self::foo´] format. Please let me know if you prefer the first and I can change them to that format.

I think we should scale it back about 30% — anything that doesn't require crate:: or self:: or std::fmt, etc could be lighter. Is that reasonable?

Just that we are on the same page.
I should handle this via the first option ([´foo´](Self::foo)), right?

I have hidden the std::fmt and crate:: calls as such.

For Self:: though this gives a bit of additional info (where is this located in relateion to what I am reading) which I whink might be helpfull.

@CommanderStorm
Copy link
Contributor Author

We can ignore the vendored path though

Read: Please remove the changes from said code?

@max-sixty
Copy link
Collaborator

Read: Please remove the changes from said code?

No need to revert! But no need to do any more on it / have strict lints etc.

@max-sixty
Copy link
Collaborator

Just that we are on the same page.
I should handle this via the first option (´foo´), right?

I have hidden the std::fmt and crate:: calls as such.

For Self:: though this gives a bit of additional info (where is this located in relateion to what I am reading) which I whink might be helpfull.

Yes! I think the current code is a great balance, thank you!

max-sixty and others added 2 commits September 27, 2024 01:10
Co-authored-by: Frank Elsinga <frank@elsinga.de>
@max-sixty
Copy link
Collaborator

I can't repro the error above re assert_ron_snapshot, but I do get this error:

cargo clippy --all-features --all-targets
warning: backticks are unbalanced
   --> insta/src/content/yaml/vendored/emitter.rs:253:5
    |
253 |   /// Strings starting with any of the following characters must be quoted.
    |  _____^
254 | | /// `:`, `&`, `*`, `?`, `|`, `-`, `<`, `>`, `=`, `!`, `%`, `@`
255 | | /// Strings containing any of the following characters must be quoted.
256 | | /// `{`, `}`, `\[`, `\]`, `,`, `#`, `\``
    | |________________________________________^
    |
    = help: a backtick may be missing a pair
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/
index.html#doc_markdown
note: the lint level is defined here
   --> insta/src/lib.rs:1:9
    |
1   | #![warn(clippy::doc_markdown)]
    |         ^^^^^^^^^^^^^^^^^^^^
    ```

@CommanderStorm
Copy link
Contributor Author

but I do get this error

Sorry, that was a botched merge from master.
Unsure if we should keep clippy::doc_markdown. Seems pretty brittle..

Apparently, the unbalanced backticks part does not properly ignore escaped backticks..

@max-sixty
Copy link
Collaborator

Unsure if we should keep clippy::doc_markdown. Seems pretty brittle..

As you wish. Definitely worth being liberal on the vendored code...

The other lint seems great fwiw, has fixed lots of issues which are now ratcheted in.


Will merge! Thank you for these @CommanderStorm

@max-sixty max-sixty merged commit ee829d8 into mitsuhiko:master Sep 27, 2024
15 checks passed
@CommanderStorm CommanderStorm deleted the better-linking branch September 27, 2024 20:09
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.

2 participants