-
Notifications
You must be signed in to change notification settings - Fork 606
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
Prep for semver bump and document current blocker #3338
Conversation
looks like CI is unhappy 😢 |
d7c8b58
to
eb8b0ac
Compare
Forgetting to |
you linked #3094 in the description, but I'm assuming that was a typo, right? 🤔 |
I believe dtolnay/semver#217 might still be blocking this |
This (partially) reverts commit 0bcb724. This partially reverts the revert (rust-lang#3051) of rust-lang#2990. While we can't bump the version yet, it is worth recovering these changes and documenting the remaining work.
eb8b0ac
to
3b9594c
Compare
@Turbo87 I've rolled back the version bump and added a comment pointing to the blocker. I've updated the PR title and description to reflect the new scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense for us to write a failing test for the blocker so that we don't update accidentally?
feel free to r=me :)
I plan to eventually land additional tests upstream. For now, this provides a convenient way to ensure we don't regress on parsing these formats.
@Turbo87 I've added a test that covers some scenarios where I observed breakage. I'll probably move this test upstream eventually, but it is worth including here for now. I've also added a command to the |
let vers = versions.select(num).load(&conn)?; | ||
test_versions(&vers); | ||
|
||
let deps = dependencies.select(req).load(&conn)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about loading all dependencies in a single query, but this is against the replica database and I expect memory usage to be somewhere around 100 MB so I think this is okay for our current scale.
for version in versions { | ||
if let Err(e) = semver::Version::parse(version) { | ||
println!("Could not parse `{}` as a semver::Version: {}", version, e); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for version in versions { | |
if let Err(e) = semver::Version::parse(version) { | |
println!("Could not parse `{}` as a semver::Version: {}", version, e); | |
} | |
} |
this looks like a duplicate to me, no?
fn test_dependency_predicates(versions: &[String]) { | ||
for version in versions { | ||
if let Err(e) = semver::VersionReq::parse(version) { | ||
println!("Could not parse `{}` as a semver::Version: {}", version, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println!("Could not parse `{}` as a semver::Version: {}", version, e); | |
println!("Could not parse `{}` as a semver::VersionReq: {}", version, e); |
|
||
if let Err(e) = semver_next::VersionReq::parse(version) { | ||
println!( | ||
"Could not parse `{}` as a semver_next::Version: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Could not parse `{}` as a semver_next::Version: {}", | |
"Could not parse `{}` as a semver_next::VersionReq: {}", |
☔ The latest upstream changes (presumably #3391) made this pull request unmergeable. Please resolve the merge conflicts. |
This partially reverts the revert (#3051) of #2990. While we can't bump the version yet, it is worth recovering these changes and documenting the remaining work.
r? @Turbo87