-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add 'alphanumeric sort' commands #4289
base: master
Are you sure you want to change the base?
Conversation
It's a short (~100L) block of code within the library and the library includes a bunch of other functions I don't think will be used in the future so I think we should vendor this block https://github.com/magiclen/alphanumeric-sort/blob/bea0284409f68407dd25b432fd901b62f6e944e7/src/lib.rs#L95-L199 |
This comment was marked as resolved.
This comment was marked as resolved.
I removed the dependency, and added a function to compare the strings. When I copied the source, I realized the handling of long numbers was incorrect (it accumulates it in a So I implemented my own function. It orders numbers with leading zeroes like the crate |
Just to note, I think that using a |
helix-term/src/commands/typed.rs
Outdated
} | ||
} | ||
|
||
#[test] |
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.
This should be moved into a
#[cfg(test)]
mod tests {
}
block at the bottom of the file.
By the way, make sure to run |
Thanks for the hints 🙂 I think I have not explained the problem with the |
fn sort_impl( | ||
cx: &mut compositor::Context, | ||
_args: &[Cow<str>], | ||
reverse: bool, | ||
alphanumeric: bool, | ||
) -> anyhow::Result<()> { |
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.
nit: you could remove both reverse
and alphanumeric
parameters and pass a single parameter called cmp_fn
which is a FnMut(&Tendril, &Tendril) -> Ordering
leaving to the caller to say how it want the ordering.
See: #8436
fragments.sort_by(match (reverse, alphanumeric) { | ||
(true, false) => |a: &Tendril, b: &Tendril| b.cmp(a), | ||
(false, false) => |a: &Tendril, b: &Tendril| a.cmp(b), | ||
(true, true) => |a: &Tendril, b: &Tendril| numeric_cmp(b, a), | ||
(false, true) => |a: &Tendril, b: &Tendril| numeric_cmp(a, b), | ||
}); |
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.
I agree with @leandrobbraga - this block is a little dense and we can make the call sites for sort_impl
a little cleaner by moving these closures to parameters of sort_impl
.
So this function would replace the reverse
parameter with a compare_fn: impl FnMut(&Tendril, &Tendril) -> std::cmp::Ordering,
param and then this block can be just fragments.sort_by(compare_fn);
.
We could also use this opportunity to drop the args
parameter which is currently unused
I was looking into this kind of thing for a personal project and I don't think that alphanumeric-sort provides true lexicographic ordering. For that, we'd want the Unicode Collation algorithm. I'm currently a little less than halfway into my naive implementation of the algorithm that would be fully compliant so I may post a PR for it in the future. Alternatively, we could use an external crate like https://crates.io/crates/feruca |
Implements #4153 with the crate mentioned in the discussion of the issue.
This adds 2 new commands,
sortn
andrsortn
, corresponding to the existing ones (sort
andrsort
), and they sort according to https://docs.rs/alphanumeric-sort/1.4.4/alphanumeric_sort/fn.compare_str.html