-
Notifications
You must be signed in to change notification settings - Fork 13.4k
make tidy-alphabetical
use a natural sort
#141311
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
base: master
Are you sure you want to change the base?
Conversation
@folkertdev We have a lot of redundant versions of this sort. See the algorithm documented in the style guide; is that algorithm sufficient here? If so, that algorithm is implemented in rustfmt. |
Oh, right, https://github.com/rust-lang/rustfmt/blob/master/src/sort.rs is probably more robust (e.g. it handles leading/repeated zeros, which my version does not). Can we achieve code sharing in a reasonable way between tidy and rustfmt? Or should I just copy-paste that implementation? |
Sharing the code is quite annoying due to how rustfmt is set up with its subtree, copying the code sounds better. It wouldn't be that bad if it diverges. |
I think because this algorithm was designed for
Contrary to alphabetical sorting;
So, idk, this algorithm has some nice features, but I think it needs some tweaking for this use case. |
1f057ed
to
f81c2f0
Compare
After looking at this a bit, I think for the sort of thing that So, I've updated the algorithm to 1) consider leading zeros and 2) perform a case-sensitive sort. In most cases that means your editor's sorting functionality will produce the correct result, except when there are numbers in the identifiers. |
// FEAT_SME_F8F32 | ||
("sme-f8f32", Unstable(sym::aarch64_unstable_target_feature), &["sme2", "fp8"]), | ||
// FEAT_SME_F16F16 | ||
("sme-f16f16", Unstable(sym::aarch64_unstable_target_feature), &["sme2"]), | ||
// FEAT_SME_F64F64 | ||
("sme-f64f64", Unstable(sym::aarch64_unstable_target_feature), &["sme"]), |
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.
Very nice improvement.
FWIW I don't use my editor to sort these things, I just do a decent enough job by eye then comply when tidy tells me to change something. So any change that looks more sensible and/or facilitates eyeball-binary-search makes me happy. |
f81c2f0
to
50f22f4
Compare
The idea here is that these lines should be correctly sorted, even though a naive string comparison would say they are not:
This is the "natural sort order".
There is more discussion in #t-compiler/help > tidy natural sort
Unfortunately, no standard sorting tools are smart enough to to this automatically (casting some doubt on whether we should make this change). Here are some sort outputs:
Disappointingly, "numeric" sort does not actually have the behavior we want. It only sorts by numeric value if the line starts with a number. The "version" sort looks promising, but does something very unintuitive if you look at the final 4 values. None of the other options seem to have the desired behavior in all cases:
r? @Noratrieb (it sounded like you know this code?)