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

feat: use semver to match required version #6066

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open

feat: use semver to match required version #6066

wants to merge 45 commits into from

Conversation

ologbonowiwi
Copy link

  • test: add test for required_version
  • chore: install semver
  • refactor: use semver to compare versions
  • fix: use exact version for comparison when no comparator is specified
  • refactor: avoid overhead by considering default behavior from semver
  • test: add complex comparisons for required_version
  • refactor: handle required_version parsing manually

Closes #6063

@ologbonowiwi ologbonowiwi mentioned this pull request Feb 9, 2024
Open
ytmimi
ytmimi reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@ytmimi ytmimi 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 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!

src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
@ologbonowiwi
Copy link
Author

changes addressed @ytmimi. Thanks for the quick feedback 😄

ytmimi
ytmimi reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@ytmimi ytmimi left a 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

Configurations.md Outdated Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
ologbonowiwi
ologbonowiwi commented Oct 8, 2024
View reviewed changes
Configurations.md Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
src/config/mod.rs Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
src/config/mod.rs Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
Configurations.md Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
Configurations.md Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
Configurations.md Outdated Show resolved Hide resolved
ytmimi
ytmimi reviewed Oct 10, 2024
View reviewed changes
Configurations.md Outdated Show resolved Hide resolved
@ologbonowiwi ologbonowiwi force-pushed the feat/#6063/use-semver-to-check-required-version branch from ea85318 to 4e15dbf Compare October 10, 2024 18:07
ytmimi
ytmimi reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

A minor note about rewording the #### Multiple version to match docs and maybe adding another test case for the wildcard match.

src/config/mod.rs
Comment on lines 1692 to 1698
// These are not allowed. '*' can't be used with other version specifiers.
#[test]
fn test_wildcard_any_with_range() {
assert!(!check_semver_version("*, <2.0.0", "1.0.0"));
assert!(!check_semver_version("*, 1.0.0", "1.5.0"));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a check_semver_version test that shows that * matches any valid version when it's used on it's own? We do already have test_required_version_wildcard_any above so maybe it's not necessary.

Configurations.md
Comment on lines 2408 to 2414
Except by `*`, any of the semver operators can be combined, being split with commas. The comparison is done using `&&` operator.

`*` can't be used with other comparators.

Since `*` has range restrictions, any comparator will override the wildcard operator. When `*` is used alone, the comparison always fails, as that's an invalid config. `*, <1.0.0` for example is invalid.

Version requirements can't contradict themselves, otherwise they'll always fail. Some examples are `"1.*, >2.0.0"`, `1.0.*, >2.0.0` and `<1.5.0, >1.10.*`, because both requirements can't be true at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could reword this slightly to hopefully make it clearer:

A comma separated list of version requirements.
The match succeeds when the current rustfmt version matches all version requirements.

The one notable exception is that a wildcard matching any version cannot be used in the list.
For example, `*, <1.0.0` will always fail.

Additionally, the version match will always fail if any of the version requirements contradict themselves.
Some examples of contradictory requirements are `1.*, >2.0.0`, `1.0.*, >2.0.0` and `<1.5.0, >1.10.*`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ytmimi ytmimi ytmimi left review comments

Assignees
No one assigned
Labels
pr-waiting-on-author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use semver to check required_version instead of comparing two strings with !=
3 participants
@ologbonowiwi @ytmimi @rustbot