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

feat(commands): sort command #1288

Merged
merged 4 commits into from
Dec 27, 2021
Merged

feat(commands): sort command #1288

merged 4 commits into from
Dec 27, 2021

Conversation

matoous
Copy link
Contributor

@matoous matoous commented Dec 18, 2021

Add basic implementation of sort command.

This PR adds sort and sort! (reverse sort) commands that sort selections. This can be used to not only sort data on separate lines (select -> split on new line Alt-s -> :sort) but also any arbitrary list (select -> select all matches, for example numbers \d+ -> :sort).

Additional functionality can be added in the future such as removing duplicates, case insensitivity, etc.


edit: provide better description for the PR

@sudormrfbin
Copy link
Member

I think it would be better to sort selections instead of lines since it gives more flexibility since the contents of the selection would be compared and reordered. It also reuses the concept of multiple cursors and selections which we want to do wherever possible.

@archseer
Copy link
Member

Note that this is also possible to do by using the pipe command to pipe each selection to sort

@matoous
Copy link
Contributor Author

matoous commented Dec 18, 2021

👋 hi, first of all, apologies for very ad-hoc PR, should have discussed it before I even opened something. I wanted to learn more about the internal workings so I tried creating something that I am currently missing from vim as it seemed like a good start.

Secondly, thanks a lot for the suggestions. I wasn't sure if my approach makes sense but your suggestions are definitely right, I will try to adjust the PR.

Few questions I was wondering about:

  • the simply functionality of sorting a few lines would be with the suggested approach: select given lines for example using view mode and x, split selection on new lines using Alt-s, and call :sort command? Do I have correct idea?

Note that this is also possible to do by using the pipe command to pipe each selection to sort

  • Does that mean that sort as a command is unnecessary or just that there could be two ways to do this?

  • Should I even fix this or rather open an issue first a discuss what the approach should be? Or we can do it here?

  • Could I please get a suggestion of how to properly write a test for such functionality?


Side note: thanks a lot for the awesome work on helix, it's something I was looking for for a long time and I enjoy it very much so far.

@dead10ck
Copy link
Member

My first reaction to this was that it is unnecessary, since you can just pipe to sort; however, on some more thought, I'm intrigued by the idea of sorting ranges. This would e.g. allow sorting the members of a list literal that don't even necessarily have to be on multiple lines.

@matoous matoous marked this pull request as ready for review December 19, 2021 09:15

fn sort_impl(
cx: &mut compositor::Context,
_args: &[Cow<str>],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this here as I assume there could be args for case-insensitivity, number sorting, etc. Maybe it might even be worth it to extract the whole implementation into some Sorter.

Copy link
Member

Choose a reason for hiding this comment

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

Not a maintainer, but I don't see that being necessary unless there were many commands sharing this same function. A single option seems fine to leave as a bool.

helix-term/src/commands.rs Show resolved Hide resolved
helix-term/src/commands.rs Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@matoous
Copy link
Contributor Author

matoous commented Dec 21, 2021

So I think this might be actually pretty useful when working over selections. Little showcase:

Screen.Recording.2021-12-22.at.0.11.13.mov

book/src/generated/typable-cmd.md Outdated Show resolved Hide resolved
helix-term/src/commands.rs Show resolved Hide resolved

fn sort_impl(
cx: &mut compositor::Context,
_args: &[Cow<str>],
Copy link
Member

Choose a reason for hiding this comment

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

Not a maintainer, but I don't see that being necessary unless there were many commands sharing this same function. A single option seems fine to leave as a bool.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
name: "sort",
aliases: &[],
doc: "Sort lines in selection.",
fun: sort,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how much appetite @archseer has for doing GNU style command arguments, but if so, this could just be a single command that takes an -r flag to reverse?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a set opinion here, I did consider it might be nice to have flag parsing for typed commands in the future.

@dead10ck
Copy link
Member

Nice work, this will be super useful!

Comment on lines +2675 to +2708
fragments.sort_by(match reverse {
true => |a: &Tendril, b: &Tendril| b.cmp(a),
false => |a: &Tendril, b: &Tendril| a.cmp(b),
});
Copy link
Member

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/std/cmp/struct.Reverse.html would make it clearer when you're reverse sorting (we already import it in the file)

Copy link
Member

Choose a reason for hiding this comment

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

How do we use it though? Reverse is a struct, so if we try to use it in a match or if/else, it complains that the types are incompatible. It looks like it was meant to use with sort_by_key and friends where there is only one thing to reference.

},
TypableCommand {
name: "sort",
aliases: &[],
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should do an alias sor which is similar to vim?

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.

6 participants