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

Use alphanumeric sorting for VisitOrder#createByName #36

Merged
merged 8 commits into from
Sep 9, 2024

Conversation

NebelNidas
Copy link
Collaborator

@NebelNidas NebelNidas commented Jun 16, 2023

Useful especially for intermediary-like names, so situations like these don't happen:
image

See the Yarn diff here: FabricMC/yarn@4ec1a6e...NebelNidas:yarn:1.21.1-alphanumeric-sort

Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Some tests would be nice, but considering its an exisitng algorithm I trust it works.

@NebelNidas
Copy link
Collaborator Author

Should I make sorting just always use the alphanumeric comparator? Then we wouldn't break the API, and there isn't much reason not to use it anyway, since performance overhead should be negligible

@liach
Copy link

liach commented Sep 23, 2023

I don't think including an outside comparator implementation under a different license is a good idea: This sort should be done on the intermediary user side as it is not beneficial to any other case.

In addition, the comparator we actuall need is way more simple than this: the implementation would simply be like Comparator.comparing(String::length).thenComparing(Comparator.naturalOrder()).

@modmuss50 modmuss50 self-requested a review September 25, 2023 22:13
@sfPlayer1
Copy link
Contributor

I have my own alphanum sorting impl somewhere..

@NebelNidas NebelNidas closed this Mar 16, 2024
@NebelNidas NebelNidas reopened this Sep 7, 2024
@NebelNidas
Copy link
Collaborator Author

NebelNidas commented Sep 8, 2024

@liach My rationale for reopening this (as discussed on Discord):

  • I very much believe MIO's default output should be as human-friendly and intuitively sorted as possible.
  • Comparing by length first, as per your suggestion, may work well for obfuscated source names (gives a ProGuard-like order), but for non-obfuscated source names this gives quite suboptimal results.
  • Providing a proper default in MIO directly helps for when we add the sorted property via Tiny v2.1, which is going to allow efficient binary searching, and having good first-party sorting raises the chances of consumers using it = less chance of a custom order being used = higher chance that the order used for writing is available at read-time.

The licensing concerns are addressed by switching to an Apache-2.0-licensed algorithm instead, we might even switch to Player's solution when he gets the remaining bugs ironed out.

@NebelNidas NebelNidas changed the title Support alphanumeric visit order Use alphanumeric sorting for VisitOrder#createByName Sep 8, 2024
@NebelNidas NebelNidas merged commit d6b7a22 into FabricMC:dev Sep 9, 2024
3 checks passed
@NebelNidas NebelNidas deleted the alphanumeric-sort branch September 9, 2024 20:05
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.

5 participants