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

models::Version: Change num field to a plain String #3650

Merged
merged 1 commit into from
May 27, 2021

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented May 26, 2021

This PR prepares for semver v1.x no longer having a diesel feature, which would make it much more complicated for us to directly deserialize the SQL column into a semver::Version field. We can deserialize it to a String instead, and only parse it into a semver::Version on demand.

We can parse it into `semver::Version` on demand instead
@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels May 26, 2021
@jtgeibel
Copy link
Member

LGTM! It seems there are 4 existing versions that cannot be parsed in semver v1.x due to a leading zero in the pre-release portion. We'll want to do a bit more testing on those when bumping semver, but this seems like the most reasonable approach.

I've tested a few things locally by adding a few version numbers of -1 to the database, which fails to parse with the current semver. When sorting in descending order, it look like any un-parsable versions will appear at the end (in JSON, the frontend does its own sorting and seems to maybe fallback to version id). Filtering bad semvers in TopVersions::from_date_version_pairs just causes the version to be blank on the search results in the worst case (currently only affecting hxgm30-client with only a single invalid version).

@bors r+

@bors
Copy link
Contributor

bors commented May 27, 2021

📌 Commit 7a5e799 has been approved by jtgeibel

@bors
Copy link
Contributor

bors commented May 27, 2021

⌛ Testing commit 7a5e799 with merge b239314...

@bors
Copy link
Contributor

bors commented May 27, 2021

☀️ Test successful - checks-actions
Approved by: jtgeibel
Pushing b239314 to master...

@bors bors merged commit b239314 into rust-lang:master May 27, 2021
@Turbo87 Turbo87 deleted the no-semver-columns branch May 27, 2021 11:14
@Turbo87 Turbo87 mentioned this pull request May 27, 2021
bors added a commit that referenced this pull request Jun 8, 2021
Update `semver` to v1.x

This PR updates the `semver` crate to the new v1.0.0 release, which is basically a complete rewrite. The `diesel` feature no longer exists, but due to #3650 we don't need it anymore.

Closes #3338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants