-
Notifications
You must be signed in to change notification settings - Fork 850
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
feat: use semver
to match required version
#6066
base: master
Are you sure you want to change the base?
feat: use semver
to match required version
#6066
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this one so soon. I'll be a little busy this coming weekend so I wanted to at least get my initial review done now. Good stuff so far!
…n' of github.com:ologbonowiwi/rustfmt into feat/rust-lang#6063/use-semver-to-check-required-version
changes addressed @ytmimi. Thanks for the quick feedback 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more notes, but we're moving in the right direction
Co-authored-by: Yacin Tmimi <[email protected]>
…n' of github.com:ologbonowiwi/rustfmt into feat/rust-lang#6063/use-semver-to-check-required-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll re-review this one again soon, just wanted to leave one quick comment on the updated docs, which I think are very user friendly!
When you have a moment could you squash all the commits down into a single commit.
Co-authored-by: Yacin Tmimi <[email protected]>
required.split(',').enumerate().for_each(|(i, required)| { | ||
let Some(comparator) = required_version.comparators.get_mut(i) else { | ||
return; | ||
}; | ||
|
||
if !required.contains('^') && comparator.op == semver::Op::Caret { | ||
comparator.op = semver::Op::Exact; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment somewhere to this section to explain that this is used to workaround the default behavior of the semver
crate. I think having that context captured in a comment for future readers of the code would be very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the check change to this?
- if !required.contains('^') && comparator.op == semver::Op::Caret {
+ if !required.starts_with'^') && comparator.op == semver::Op::Caret {
comparator.op = semver::Op::Exact;
}
also, to avoid any confusion between the required
passed to the check_semver_version
and the required
in the body of the closure maybe one of those variables should be renamed?
Co-authored-by: Yacin Tmimi <[email protected]>
required_version
semver
semver
to compare versionssemver
required_version
parsing manuallyCloses #6063