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.

Sign up for GitHub

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

Jump to bottom

Changed description of caret character in docs #12209

Closed
wants to merge 5 commits into from
Closed

Changed description of caret character in docs #12209

wants to merge 5 commits into from

Conversation

gautamprikshit1
Copy link

@gautamprikshit1 gautamprikshit1 commented May 31, 2023 •
edited
Loading

Changed the description of caret character in dependency docs. Fixes #12112

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

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 May 31, 2023
weihanglo
weihanglo reviewed May 31, 2023
View reviewed changes
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.

Thanks for being interested in contributing to this project! I guess you're fixing #12112. As a good practice, contributors are encouraged to follow the process of submitting a PR in Cargo Contributor Guide, as well as the guideline in the pull request template.

I would suggest providing a minimal description for this pull request.

weihanglo
weihanglo reviewed May 31, 2023
View reviewed changes
src/doc/src/reference/specifying-dependencies.md Outdated
@@ -56,7 +56,7 @@ using special operators, though it shouldn't be necessary most of the time.
### Caret requirements

**Caret requirements** are an alternative syntax for the default strategy,
`^1.2.3` is exactly equivalent to `1.2.3`.
`^1.2.3` means the same thing as `1.2.3`, which allows [SemVer]: https://semver.org compatible updates.
Copy link
Member

Choose a reason for hiding this comment

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

This combines suggestion from me and ehuss in #12112. What do you think?

Suggested change
`^1.2.3` means the same thing as `1.2.3`, which allows [SemVer]: https://semver.org compatible updates.
For instance, `log = "^1.2.3"` means the same thing as `log = "1.2.3",
which allows [SemVer] compatible updates.

weihanglo
weihanglo requested changes Jun 1, 2023
View reviewed changes
src/doc/src/reference/specifying-dependencies.md Outdated
@@ -56,7 +56,8 @@ using special operators, though it shouldn't be necessary most of the time.
### Caret requirements

**Caret requirements** are an alternative syntax for the default strategy,
`^1.2.3` is exactly equivalent to `1.2.3`.
For instance, `log = "^1.2.3"` means the same thing as `log = "1.2.3"`,
which allows [SemVer]: https://semver.org compatible updates.
Copy link
Member

Choose a reason for hiding this comment

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

The link is unnecessary I believe?

Suggested change
which allows [SemVer]: https://semver.org compatible updates.
which allows [SemVer] compatible updates.

weihanglo
weihanglo approved these changes Jun 1, 2023
View reviewed changes
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.

Thanks.

I am going to merge this once CI passes. If you manage to squash commits into one before that, feel free to do so.

@weihanglo
Copy link
Member

Sorry but no that is not what I meant. I suggested squashing for keeping the git history meaningful. It's not a hard requirement but this PR is better to fit in a single commit.

Some useful resources:

BTW, communication is essential in open source project collaboration. Feel free to comment here or ask anything.

@gautamprikshit1
Copy link
Author

I just updated the branch, I will squash the commits soon.
Will join the zulip chat for communication on further issues. I appreciate the resources and feedback.

LingMan
LingMan reviewed Jun 5, 2023
View reviewed changes
src/doc/src/reference/specifying-dependencies.md
@@ -56,7 +56,8 @@ using special operators, though it shouldn't be necessary most of the time.
### Caret requirements

**Caret requirements** are an alternative syntax for the default strategy,
Copy link

Choose a reason for hiding this comment

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

You're starting a new sentence in the next line, so the comma at the end of this one should be turned into a period.

@weihanglo
Copy link
Member

@rustbot author

@gautamprikshit1, when this is ready for the next round of review, use @rustbot ready to notify reviews.

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2023
@gautamprikshit1
Copy link
Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jun 6, 2023
@gautamprikshit1
Copy link
Author

@weihanglo couldn't squash commits due to the branch updates between commits and hence large number of commits

@weihanglo
Copy link
Member

@gautamprikshit1, it seems that you still did it wrong.

d9434e0 is not what LingMan suggests. We probably need to drop that commit. Besides, the commits are still piled up in this pull request. Have you read those aformentioned materials #12209 (comment)? They should provide a decent amount of understanding :)

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2023
@gautamprikshit1
Copy link
Author

@weihanglo I don't think it will make sense to have a sentence start from which in this case. Having period at the end will signify a new sentence

@LingMan
Copy link

LingMan commented Jun 6, 2023

@gautamprikshit1 My suggestion was to change , For to . For, not , which to . which. It's one line earlier.

@weihanglo
Copy link
Member

Due to inactivity and to reduce the review queue, this will be closed shortly. The patch should be relatively easy. Feel free to submit a new one when the above Git and grammar issues are resolved.

@weihanglo weihanglo closed this Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LingMan LingMan LingMan left review comments

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

subtle documentation ambiguity
4 participants
@gautamprikshit1 @rustbot @weihanglo @LingMan