Skip to content

Remove mention of possibility to specify the MSRV with a tilde/caret #6386

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

Merged
merged 2 commits into from
Nov 27, 2020

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Nov 26, 2020

As @taiki-e explained in #6379 (comment), mentioning this might be problematic.

changelog: none

@rust-highfive
Copy link

r? @ebroto

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 26, 2020
@@ -214,7 +214,8 @@ fn main() {
}
```

Tilde/Caret version requirements (like `^1.0` or `~1.2`) can be specified as well.
You can also omit the patch version when specifying the MSRV, so `msrv = 1.30`
is equivalent to `msrv = 1.30.0`.
Copy link
Contributor

Choose a reason for hiding this comment

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

@flip1995 I think omitting the patch version might not work. Parsing 1.30 doesn't throw an error but, comparisons with the Lint's MSRV are not correct. Specifying the MSRV as ^1.30 works though

I just ran the snippet below

use semver::{Version, VersionReq};

fn main() {
  const MANUAL_STRIP_MSRV: Version = Version {
      major: 1,
      minor: 45,
      patch: 0,
      pre: Vec::new(),
      build: Vec::new(),
  };
  match VersionReq::parse("1.30") {
    Ok(vReq) => {
      if !vReq.matches(&MANUAL_STRIP_MSRV) {
        println!("Will lint!")
      } else {
        println!("Won't lint!")
      }
    },
    Err(_) => println!("Error")
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's bad. We have to fix this and improve our testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

After playing around a bit, I think, that VersionReq is broken 😅 Look at this: Playground

Not what I would expect for the output. I think @taiki-e is right and we should write our own version parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

@flip1995 shall we also open an issue on semver?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, can you please do this. I'm currently writing a RustVersion crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ebroto using Version for both is what we did initially but then thought that being able to ignore the patch version would be a nice to have since major changes aren't introduced in patches

I've opened an issue here: dtolnay/semver#221, maybe you guys can add your observations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done: https://crates.io/crates/rustc-semver

@flip1995 I can work on switching to this new crate instead of using semver

Copy link
Member Author

Choose a reason for hiding this comment

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

@flip1995 I can work on switching to this new crate instead of using semver

That would be great. I want to get this crate to 100% code coverage though. This will be my weekend project.

Copy link
Member Author

Choose a reason for hiding this comment

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

@suyash458 rustc-semver now has 100% code coverage (tested in CI) and has a 1.0.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@suyash458 rustc-semver now has 100% code coverage (tested in CI) and has a 1.0.0 release.

And it's not even the weekend yet 😄

@ebroto
Copy link
Member

ebroto commented Nov 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2020

📌 Commit 6eb2c27 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Nov 27, 2020

⌛ Testing commit 6eb2c27 with merge f9b8a59...

@bors
Copy link
Contributor

bors commented Nov 27, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing f9b8a59 to master...

@bors bors merged commit f9b8a59 into master Nov 27, 2020
@flip1995 flip1995 deleted the flip1995-patch-1 branch November 27, 2020 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants