fix(java): sort order for shorthand versions#8197
Conversation
Summary of ChangesHello @roele, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue with the sorting of Java versions, particularly when shorthand notations are employed. The changes refine the internal sorting mechanism by simplifying the version comparison key and ensuring consistent treatment of shorthand versions, which resolves potential discrepancies in the displayed order of available Java installations. A new end-to-end test has been introduced to confirm the correct sorting behavior. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the sort order for Java shorthand versions (e.g., java@21) to match their corresponding full vendor versions (e.g., java@temurin-21). The fix simplifies the sorting logic by removing custom build number extraction and relying on the standard Versioning crate for proper version comparison.
Changes:
- Removed custom build number extraction logic that was incorrectly parsing version strings
- Changed shorthand versions to always be treated as JDK (not JRE) in sorting
- Removed unused
std::str::FromStrimport - Added test to verify shorthand versions have correct sort order
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/plugins/core/java.rs | Simplified version sorting by removing build_num extraction and treating shorthand versions as JDK |
| e2e/core/test_java_shorthand_vendor_slow | Added test case to verify sort order consistency between shorthand and vendor-specific versions |
Comments suppressed due to low confidence (1)
e2e/core/test_java_shorthand_vendor_slow:17
- The test logic doesn't verify sort order. The pattern created with
paste -sd '|'creates a regex alternation that only checks if any temurin version appears anywhere in the java@21 output, not whether they appear in the same order. To properly test sort order, consider comparing the full ordered lists or checking specific ordering relationships between versions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request aims to fix the sort order for Java shorthand versions. The change to correctly identify shorthand versions as JDKs is a good improvement. However, I've identified a regression and another potential issue in the sorting logic, along with a lifetime concern for the sorting key:
- The removal of
build_numfrom the sort key will cause incorrect sorting for versions with build metadata (e.g.,+12). - The current sorting logic prioritizes non-shorthand versions over shorthand versions, which seems contrary to the expected behavior.
- The key returned by
sorted_by_cached_keymust consist of owned values to prevent lifetime errors.
I've provided a detailed comment with a suggested fix for these issues in src/plugins/core/java.rs. Additionally, I've suggested an improvement to the test case to ensure it properly validates the sort order.
544bbe1 to
8fcaf6e
Compare
|
bugbot run |
### 🚀 Features - **(mcp)** add run_task tool for executing mise tasks by @joaommartins in [#8179](#8179) - **(node)** suggest setting node.flavor if the flavor is not found in mirror by @risu729 in [#8206](#8206) ### 🐛 Bug Fixes - **(java)** sort order for shorthand versions by @roele in [#8197](#8197) - **(node)** migrate env vars to settings by @risu729 in [#8200](#8200) - **(registry)** apply overrides in shims by @risu729 in [#8199](#8199) - migrate MISE_CARGO_BINSTALL_ONLY to settings by @risu729 in [#8202](#8202) ### 📚 Documentation - **(doctor)** fix subcommand in an example by @risu729 in [#8198](#8198) ### 📦 Registry - add github backend for typst by @3w36zj6 in [#8210](#8210) ### Chore - **(test)** disable flaky Forgejo e2e test by @jdx in [#8211](#8211)
Co-authored-by: mise-en-dev <release@mise.jdx.dev>
Co-authored-by: mise-en-dev <release@mise.jdx.dev>
### 🚀 Features - **(mcp)** add run_task tool for executing mise tasks by @joaommartins in [jdx#8179](jdx#8179) - **(node)** suggest setting node.flavor if the flavor is not found in mirror by @risu729 in [jdx#8206](jdx#8206) ### 🐛 Bug Fixes - **(java)** sort order for shorthand versions by @roele in [jdx#8197](jdx#8197) - **(node)** migrate env vars to settings by @risu729 in [jdx#8200](jdx#8200) - **(registry)** apply overrides in shims by @risu729 in [jdx#8199](jdx#8199) - migrate MISE_CARGO_BINSTALL_ONLY to settings by @risu729 in [jdx#8202](jdx#8202) ### 📚 Documentation - **(doctor)** fix subcommand in an example by @risu729 in [jdx#8198](jdx#8198) ### 📦 Registry - add github backend for typst by @3w36zj6 in [jdx#8210](jdx#8210) ### Chore - **(test)** disable flaky Forgejo e2e test by @jdx in [jdx#8211](jdx#8211)
No description provided.