rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

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

Open wesleymatosdev opened this issue 1 year ago • 5 comments

My plan is to use semver library to achieve so.

The idea is to change https://github.com/rust-lang/rustfmt/blob/041f11315923b0e4b0328b3bb6366de9148c7b6b/src/config/mod.rs#L217 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
- [x] Add tests for current implementation
- [x] Implement new version with `semver`, making sure it does not break old (current) behavior
- [x] Add "stress test" checking multiple definitions to the value (using fancy operators such as `>`, `>=`, `<`, `<=`, `^`, `8`, `~`, etc.), and ensuring it'll work

wesleymatosdev avatar Feb 08 '24 17:02 wesleymatosdev

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 ^.

boozook avatar Feb 08 '24 18:02 boozook

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.

wesleymatosdev avatar Feb 08 '24 18:02 wesleymatosdev

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

wesleymatosdev avatar Feb 08 '24 18:02 wesleymatosdev

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.

ytmimi avatar Feb 08 '24 18:02 ytmimi

#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: https://github.com/rust-lang/rustfmt/pull/6066/commits/73e75adc63e337c048cfdac380710b7dd1939ba7, 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 :smile:

wesleymatosdev avatar Feb 09 '24 01:02 wesleymatosdev