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

semver-parser 0.10.0 #209

Merged
merged 15 commits into from Sep 24, 2020
Merged

semver-parser 0.10.0 #209

merged 15 commits into from Sep 24, 2020

Conversation

hone
Copy link
Contributor

@hone hone commented Jul 22, 2020

This combines the changes from mikrostew/semver@new-parser and the current master branch which makes it compatible with semver-parser 0.10.0 that hasn't quite been released. In order for the tests pass, it patches the semver-parser dependency to use GitHub.

Tracking Items Left:

Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This is amazing, and we should :shipit: . Thanks so much again for doing this.

I have one quick question, since you've been in this code lately, but IMHO we should cut the semver-parser release, update this to use it, and cut a new semver release, probably over the next few days.

@@ -10,7 +10,8 @@ fn test_regressions() {
("0.1.0.", VersionReq::parse("0.1.0").unwrap()),
("0.3.1.3", VersionReq::parse("0.3.13").unwrap()),
("0.2*", VersionReq::parse("0.2.*").unwrap()),
("*.0", VersionReq::any()),
// TODO: this actually parses as '*' now, not sure if that's OK
// ("*.0", VersionReq::any()),
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I am actually not sure what the difference here is between "parses as *" and Any, that is, they should be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was comparing it on https://semver.npmjs.com/, they both resolve to the same set of versions. It's now not "deprecated". The question is if we care about making it deprecated (want to discourage people from using this)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, the deprecation. Thanks :) It is probably fine to un-deprecate it, but I'd like to triple check against semver/semver#584 real quick to see if it's acceptable in that grammar....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also some other TODOs we need to either clean up in this PR or another one before we ship a new version of both semver and semver-parser that @mikrowstew left behind.

Copy link

Choose a reason for hiding this comment

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

I think semver/semver#584 disallows it, but fwiw, node-semver parses it as * as well. If y'all move forward with doing it that way, we should update the spec to make that official.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@isaacs that sounds good to me.

@mikrostew
Copy link
Contributor

Wow, this is great! Thank you so much for seeing this through to the end ❤️

@steveklabnik
Copy link
Contributor

@hone checking on this; did you want me to merge it now, or did you want to try to tackle the TODOs first? It wasn't clear to me :)

@hone
Copy link
Contributor Author

hone commented Aug 4, 2020

@steveklabnik we can merge this in now and I can just do the other TODOs in a separate PR. We would just want to address them before releasing either crate.

@hone
Copy link
Contributor Author

hone commented Aug 4, 2020

@mikrostew thanks for doing all the hard legwork here. :)

@hone
Copy link
Contributor Author

hone commented Sep 11, 2020

@steveklabnik thoughts on renaming Compat::Node to Compat::NPM or Compat::Cargo to Compat::Rust?

@steveklabnik
Copy link
Contributor

Yeah it should probably be Compat::Npm. Technically these are associated with the tools, not the languages.

hone added a commit to hone/semver-parser that referenced this pull request Sep 14, 2020
Technically these are associated with the tools, not the languages.
dtolnay/semver#209 (comment)
Technically these are associated with the tools, not the languages.
dtolnay#209 (comment)
@steveklabnik
Copy link
Contributor

(Going to push a commit to un-patch semver-parser after this. Thanks for all the work, @hone <3)

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 this pull request may close these issues.

None yet

4 participants