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

Add limited support for :sort #1342

Merged
merged 2 commits into from
Feb 28, 2017
Merged

Conversation

jordan-heemskerk
Copy link
Contributor

  • Works on a range
  • ! option is supported to reverse the sort
  • Other options not supported at this time

Fixes: #1341

@xconverge
Copy link
Member

xconverge commented Feb 28, 2017

Hmmm, you could have used editor.action.sortLinesAscending and editor.action.sortLinesDescending I think?

Then you could just add a file like this

https://github.com/VSCodeVim/Vim/blob/master/src/cmd_line/commands/quit.ts

but for sort, and have it call those 2 commands

@jordan-heemskerk
Copy link
Contributor Author

@xconverge That'd be super nice not to have to roll my own sorting routine. Is there a way we can support a line range whilst doing that?

For example: 1,3sort, see Line 41 in the new test file.


export function parseSortCommandArgs(args : string) : node.SortCommand {

let reverse_ = false;
Copy link
Member

Choose a reason for hiding this comment

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

Why the trailing underscore? Also you could write const reverse = args !== null && args.indexOf("!") >= 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, my bad. Will update the conditional as well.

 - Works on a range
 - ! option is supported to reverse the sort
 - Other options not supported at this time

Fixes: VSCodeVim#1341
@xconverge
Copy link
Member

@jordan-heemskerk I think you would have to set the selection to contain those lines and then run the command

Might just be easier to keep the homegrown one though since a sort isn't too complicated

@jordan-heemskerk
Copy link
Contributor Author

@xconverge yeah, my thinking as well if that is the case. Especially since that sorting routine may grow more complex to support the full option list of :sort.

@xconverge
Copy link
Member

the failing test isn't you

@jordan-heemskerk
Copy link
Contributor Author

Yeah looks like master is in a bad state. I'll let you guys merge this once its happy again.

const reverse = args !== null && args.indexOf("!") >= 0;

return new node.SortCommand({
reverse: reverse,
Copy link
Member

Choose a reason for hiding this comment

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

Fun fact: { reverse: reverse } can be better written in ES6 as { reverse }. :)

@johnfn
Copy link
Member

johnfn commented Feb 28, 2017

Heh I forgot I can make little tweaks directly from Github. That's a new feature :)

Alright, this LGTM! Thanks for the PR, @jordan-heemskerk 😄

@johnfn johnfn merged commit bf96cd2 into VSCodeVim:master Feb 28, 2017
This pull request was closed.
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.

3 participants