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 line comment operator #1506

Merged
merged 5 commits into from
Apr 16, 2017
Merged

Add line comment operator #1506

merged 5 commits into from
Apr 16, 2017

Conversation

kimfiedler
Copy link
Contributor

@kimfiedler kimfiedler commented Apr 13, 2017

This PR adds functionality similar to tpope's vim-commentary. As it didn't require a lot of work I thought I would just create a PR instead of opening an issue / feature request.

It can toggle line comment the current line using gbb or toggle comments on the target of a motion i.e. gb5j to toggle line comments on current line and the following next four lines or gbap to toggle line comments in a paragraph etc.

It uses the built-in Toggle Line Comment functionality of VSCode to determine how to apply/unapply comments.

A few questions:

  • I would have liked to use gc as the original Vim plugin but since this is already used in this extension I settled for gb. Thought on this?
  • Since VSCode's Toggle Line Comment relies on the current editors language mode to determine how to apply comments I could not find an good way to add tests for this as it does nothing in Plain Text mode. Any advice on the best way to add a test case that runs under another language mode (for example JavaScript)?

Let me know what you think. Thanks!

@xconverge
Copy link
Member

You could then remap gc to send gb in your settings

Also if we allow remapping particular actions in the future this would be solved, you. Could map gc to CommentLine action

@kimfiedler
Copy link
Contributor Author

Remapping gc to send gb sounds good to me! Tried it out and it works like you'd expect.

Are you saying that this PR wasn't needed if we could remap to actions? I guess there should need to be some way to handle selection based on motion and cursor position after the action for that to work out...

If you think we should move forward with this PR I can try to look in to how to add a few tests. My initial thoughts are to add the tests to test/mode/modeNormal.test.ts and find a way to set the languageId on the textEditor for these specific tests.

@xconverge
Copy link
Member

xconverge commented Apr 13, 2017

No that is not what I was saying, I was saying now that the action atleast exists in the codebase, later on it can be explicitly remapped.

instead of before: gc after: gb it will be before: gc after: CommentOperator

I am all for moving forward with this, it has been asked for quite a few times, I would even be interested in you making a CommentBlockOperator using the other native vscode command for commenting blocks. These 2 actions would be very useful to have in the code base, how we remap them and trigger them is secondary as long as there is A WAY right now

I like it!

@kimfiedler
Copy link
Contributor Author

Ah I see what you mean now.

Great! I've added some tests for the comment operator. There is no API to change the language of an editor so instead I decided to change the setupWorkspace function to accept a file extension for the file created for a test case.

I'd be happy to create the CommentBlockOperator as well. I won't have time the next couple of days but I'll get to it some time next week.

@xconverge
Copy link
Member

I am a bit torn on the gc/gb thing. Anyone who uses this will want gc :/ @johnfn Any ideas here?

@johnfn
Copy link
Member

johnfn commented Apr 16, 2017

Yeah, kind of a namespacing bummer there.

I suppose we could have an option. By default, let's put this on gb as to not break any existing workflows. Later we can add an option to say which addon gets gc.

@johnfn
Copy link
Member

johnfn commented Apr 16, 2017

I'd say if you can add a little blurb about this into the README, and maybe include your remapper, we should be good to go here.

@kimfiedler
Copy link
Contributor Author

Yeah it's a bit unfortunate that gc was already taken here...

I've added the BlockCommentOperator as well as a small section in README.md that describes the usage and how to remap if you want to.

Please take another look at this and let me know if you want me to change anything.

Thanks!

@xconverge
Copy link
Member

This is really awesome, thanks a lot @fiedler

@xconverge xconverge merged commit 2ff7807 into VSCodeVim:master Apr 16, 2017
@kimfiedler kimfiedler deleted the comment branch April 16, 2017 17:56
@Chillee
Copy link
Member

Chillee commented May 18, 2017

@fiedler It kinda seems like the features you implemented align more with http://www.vim.org/scripts/script.php?script_id=1173.

Is that right?

Actually, it seems like kind of a mix. gC seems to be from TComment, but otherwise, it's more like vim-commentary.

@kimfiedler
Copy link
Contributor Author

Hmm. It's been a while since I used Vim so it might actually have been TComment. I did this implementation based on the behaviour I am use to in other editors. So it is likely that it is indeed a mix of multiple implementations.

@Chillee
Copy link
Member

Chillee commented May 22, 2017

Well, it doesn't too much :P Commentary is the most popular plugin (and it's also super simple), so I don't see any problem with that.

Also, in a PR I merged recently, I swapped gc and gb, so the comment operator has been restored to its true form :)

@kimfiedler
Copy link
Contributor Author

Nice 🎉

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.

4 participants