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

Clarify note in example #15054

Merged
merged 1 commit into from
Jan 12, 2025
Merged

Clarify note in example #15054

merged 1 commit into from
Jan 12, 2025

Conversation

AeonSolstice
Copy link
Contributor

What does this PR try to resolve?

I was reading the docs to see how to specify a version to use from a git dependency, and came across this section and got really confused as I did not know what N.B. meant so the following did not read well for me: N.B. that if .... I had to look it up and then realized that I had wasted time that did not need to be wasted. Thus this PR aims to prevent the time of future readers from being wasted.

How should we test and review this PR?

Try to read aloud the old version and the new one, as well as the given alternatives to see which would make most sense to the largest number of people.

Additional information

A simple fix would be to just remove "that" as this reads better: N.B. if a version doesn't match, Cargo will fail to compile!

Other variations

  • Note that if a version doesn't match, Cargo will fail to compile!
    • might be confused as Notice that if a version doesn't match, Cargo will fail to compile!
  • Move the comment out to a reworded sentence right below the code block and perhaps also link to the The role of the version key section above which clearly states:

The version key does not affect which commit is used when Cargo retrieves the git dependency, but Cargo checks the version information in the dependency’s Cargo.toml file against the version key and raises an error if the check fails.

@rustbot
Copy link
Collaborator

rustbot commented Jan 12, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2025
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didn't even know what that really means until today 😆

@weihanglo weihanglo enabled auto-merge January 12, 2025 00:12
@weihanglo weihanglo added this pull request to the merge queue Jan 12, 2025
Merged via the queue into rust-lang:master with commit f15df8f Jan 12, 2025
21 checks passed
@AeonSolstice AeonSolstice deleted the patch-1 branch January 12, 2025 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants