Skip to content

Improve invalid semver string handling #3304

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 3 commits into from
Feb 25, 2021
Merged

Conversation

plaets
Copy link
Contributor

@plaets plaets commented Feb 16, 2021

I placed the checks so that "invalid version" has higher priority than "first version" but lower than "yanked".
I also modified css of the version list so that long versions do not overflow from tooltips.
Before
image
After
image

Fixes #3294

@rust-highfive
Copy link

r? @jtgeibel

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

@Turbo87
Copy link
Member

Turbo87 commented Feb 16, 2021

looks great. could you check if it's possible to add a few tests for these cases? 🙏

@plaets plaets force-pushed the issue-3294 branch 5 times, most recently from a0f3296 to 91c2072 Compare February 17, 2021 12:47
@plaets
Copy link
Contributor Author

plaets commented Feb 17, 2021

I added some tests, but honestly, I'm not very familiar with ember so I'm not entirely happy with the results. If you have any comments, anything I should or should not test, please let me know.
Main issues:

  • I couldn't figure out how to test the tooltip text, I tried this with [data-test-release-track-title] as the selector, but it just fails to find the element.
  • I did not add the test-max-version-example-crate case to happy path, mirage/serializers/crate fails to sort arrays with invalid versions which causes "Ember Data Request GET /api/v1/crates/crate-0 returned a 500 Payload" and I'm not sure how to solve that.
  • Is components/version-list-row-test.js a good name/path? ember g component-test version-list/row would generate the test files in a completely new path and I wasn't sure which one to pick.

And sorry for force-push spam, I forgot to remove/check some stuff before pushing.

@bors
Copy link
Contributor

bors commented Feb 19, 2021

☔ The latest upstream changes (presumably #3263) made this pull request unmergeable. Please resolve the merge conflicts.

…id/versions`

* Add a tooltip message and a new icon to components/version-list/row
* Handle null returned from `semverParse` in models/version
* Add `loose` option to `semverParse` (https://github.com/npm/node-semver#functions)
* Add tests for version-list/row
* Add tests for invalid/non-standard semver strings to models/version
  tests
* Add loose option to more node-semver calls
@Turbo87 Turbo87 changed the title Fix #3294 - handle invalid semver strings in crates/:crate_id/versions Improve invalid semver string handling Feb 25, 2021
@Turbo87
Copy link
Member

Turbo87 commented Feb 25, 2021

looks great, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2021

📌 Commit aebe476 has been approved by Turbo87

@Turbo87 Turbo87 added A-frontend 🐹 C-bug 🐞 Category: unintended, undesired behavior labels Feb 25, 2021
@bors
Copy link
Contributor

bors commented Feb 25, 2021

⌛ Testing commit aebe476 with merge 8363da4...

@bors
Copy link
Contributor

bors commented Feb 25, 2021

☀️ Test successful - checks-actions
Approved by: Turbo87
Pushing 8363da4 to master...

@bors bors merged commit 8363da4 into rust-lang:master Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-bug 🐞 Category: unintended, undesired behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'prerelease' of null
5 participants