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

Use semver to check required_version instead of comparing two strings with != #6063

Open
3 tasks done
ologbonowiwi opened this issue Feb 8, 2024 · 5 comments · May be fixed by #6066
Open
3 tasks done

Use semver to check required_version instead of comparing two strings with != #6063

ologbonowiwi opened this issue Feb 8, 2024 · 5 comments · May be fixed by #6066

Comments

@ologbonowiwi
Copy link

ologbonowiwi commented Feb 8, 2024

My plan is to use semver library to achieve so.

The idea is to change

if version != required_version {

including the comparison tools provided by semver.

This will allow complex comparisons such as

required_version = ">=1.2.3, <1.8.0"

instead of only plain files.

I'm willing to work on it, btw.

Opening it as an effort to contribute to turning required_version into a stable flag.

Tasks

@boozook
Copy link

boozook commented Feb 8, 2024

I suppose there is also could be point like "produce correct semver-value for generated config" and should be decided what it should be - exact strict with =, or range or just ^.

@ologbonowiwi
Copy link
Author

ologbonowiwi commented Feb 8, 2024

My preferred path would be, if 1.34 is on the required_version, should return true to any version above 1.35, as this is the default behavior for rust-version, as described on The Cargo Book

rust-version — The minimal supported Rust version.

But as currently the required_version key compares equality (any different version, above or below will throw error), we can keep as it is for plain version, but allow different comparison strings, e.g.: as 1.34 breaks for 1.35, but ^1.34/>=1.34/>1.34 not.

@ologbonowiwi
Copy link
Author

ologbonowiwi commented Feb 8, 2024

produce correct semver-value for generated config

Agree totally, but not sure how this would differentiate from typing --version and copy/paste it, and not sure if this should be done in this issue. I didn't find any kind of init command (something like cargo fmt init) to generate the config file, but if we have so, then by default this could add the required version.
One possibility to it'd generate a ><current_version> <<next_major>, so user can use any minor/patch above the current version, but can't use a major version above

@ytmimi
Copy link
Contributor

ytmimi commented Feb 8, 2024

I didn't find any kind of init command (something like cargo fmt init) to generate the config file, but if we have so, then by default this could add the required version.

@ologbonowiwi you can run rustfmt --print-config default, and rustfmt will write a default toml file to stdout.

and not sure if this should be done in this issue.

Agreed. Deciding that can likely wait till later.

@ologbonowiwi ologbonowiwi linked a pull request Feb 9, 2024 that will close this issue
@ologbonowiwi
Copy link
Author

#6066 addressing it. Needed to change the behavior as semver by default, consider that a ^ exists. If we want to consider a =$VERSION when the value is $VERSION, I give it a shot here: 73e75ad, but I've deleted it as it looked pretty ugly.

Please let me know if you have any thoughts/feedbacks on the PR. And thanks for the opportunity of contributing to this fantastic project 😄

ologbonowiwi added a commit to ologbonowiwi/rustfmt that referenced this issue Feb 9, 2024
ologbonowiwi added a commit to ologbonowiwi/rustfmt that referenced this issue Feb 9, 2024
…n' of github.com:ologbonowiwi/rustfmt into feat/rust-lang#6063/use-semver-to-check-required-version
ologbonowiwi added a commit to ologbonowiwi/rustfmt that referenced this issue Feb 9, 2024
…n' of github.com:ologbonowiwi/rustfmt into feat/rust-lang#6063/use-semver-to-check-required-version
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 a pull request may close this issue.

3 participants