Add hook functionality, update docs #23

gegoune · 2021-07-10T09:27:54Z

implement hook setting allowing users to pass in arbitrary
function to call before reading commentstring
formats README file with prettier and fixes all issues reported by
markdownlint
use local variable for vim.api
replace vim.api.nvim_command with native API
use <Cmd> instead of : in key mappings to avoid changing modes

Closes #6

gegoune · 2021-07-10T09:29:36Z

@terrortylor This PR is missing tests. I have never written one before and am not sure how to do that. Some guidance would be really appreciated. Wasn't sure what to call hook setting. Maybe something more descriptive such as pre_comment_hook?

terrortylor · 2021-07-23T13:22:29Z

Sorry been awol.
Sure, take a look at the tests in the tests directory, and the Makefile for how to run.
You will need plentary (https://github.com/nvim-lua/plenary.nvim)

Suggestion for a test would be to create a clouse function and set that in the hook that modified a variable local to the test (maybe a counter), comment out some code, toggle it and check that that the variable is modified as expected.

You've some very opinionated readme changes there too ;)
(I don't really mind they are all sensible!)

EDIT:
Actually a lot of the README style changes seem pointless, and I'm not really on board with them

terrortylor · 2021-07-23T13:27:03Z

lua/nvim_comment.lua

-  vim.api.nvim_call_function("execute", {vim_func})
-  vim.api.nvim_command("command! -range CommentToggle lua require('nvim_comment').comment_toggle(<line1>, <line2>)")
+  vim.g.loaded_text_objects_plugin = 1
+  api.nvim_exec([[


This isn't 0.5 safe... FYI why it's done in this backwards way..
Whilst neovim 0.5 is now released it's not available on all distro's so changing this may be a breaking change for some users, best not to put this change in just yet.

This still holds IMO

Reverted. Although not sure how important pre 0.5 compatibility is. You can just require 0.5 as a minimum.

one of the things I wanted was support for <0.5 as selfishly I can use this one some work servers ;P

terrortylor · 2021-07-23T13:31:12Z

lua/nvim_comment.lua

 }

 function M.get_comment_wrapper()
+  if type(M.config.hook) == 'function' then


I'd actually suggest moving this to line 97 before get_comment_wrapper is set
whilst for this use case it's probably to be used to override commentstring value, it could be for somehitng else like changing the comment highlight value.

Good point, did not consider other use cases. Have moved to as instructed.

terrortylor · 2021-07-23T13:31:54Z

README.md

+```lua
+require('nvim_comment').setup({
+  hook = function()
+    if vim.api.nvim_buf_get_option(0, "filetype") == "vue" then


terrortylor · 2021-07-23T13:34:25Z

README.md

-* `CommentToggle` comment/uncomment current line
-* `67,69CommentToggle` comment/uncomment a range
-* `'<,'>CommentToggle` comment/uncomment a visual selection
+- `CommentToggle` comment/uncomment current line


Why change these?

All README changes were dictated by linter. I think in above snippet * is redundant. It's either * or - list.

ah I don't use a markdown linter, shame on me.

terrortylor · 2021-07-23T13:34:48Z

README.md

-** `gc4j` comment/uncomment 4 lines below the current line
+- `gcc` comment/uncomment current line, this does not take a count, if you want
+  a count use the `gc{count}{motion}`
+- `gc{motion}` comment/uncomment selection defined by a motion: \*\*As lines are


again why change these list types/

I am not sure if your intention here was to have indented list, ** does not do that though.
Without my changes this block looks like:

with them it changes to

Ah, just noticed that I have omitted one ** there. Will hold off fixing it for now until you give me green light. :)

terrortylor · 2021-07-23T13:35:20Z

README.md


-## Configure
+### Configure


happy with current heading level,

It was reported by linter as skipping heading levels.

Only unwanted side effect here is that GitHub generates link in TOC, that might not be something you want as it gets populated. Perhaps a list then?

not a bad point actually, alright fair enough

terrortylor · 2021-07-23T13:36:01Z

README.md

 }
 ```

-**Ignore Empty Lines**
+#### Ignore Empty Lines


again happy with these as just bold

Was reported by linter as something along the line: emphasis used in place of header. I agree with linter as emphasis such as bold text probably fit more within the sentence to emphasise it's part.

terrortylor · 2021-07-23T13:36:42Z

README.md


 ```lua
 -- Assumes this is being run in the context of the filetype...
 vim.api.nvim_buf_set_option(0, "commentstring", "# %s")
 ```

-# Installation
+## Installation


happy with current heading level

ignore this then based on your TOC comment

Made it a list, not sure if it's good enough now. Perhaps doesn't even need to be a list. Detail though.

mvllow · 2021-07-23T16:49:57Z

Really looking forward to this getting merged, can we just remove any unnecessary doc changes? That should be it's own PR anyways imo.

- implement `hook` setting allowing users to pass in arbitrary function to call before reading `commentstring` - format README file with prettier and fix all issues reported by markdownlint - use local variable for `vim.api` - use `<Cmd>` instead of `:` in key mappings to avoid changing modes

gegoune · 2021-07-25T15:00:36Z

Still missing tests, tried to give it a go but never used plenary and wrote tests, do you mind helping out a bit?

terrortylor · 2021-07-27T08:35:55Z

hey, regarding tests I can help for sure!
I'm super busy atm but maybe we can do a pair session?
I'll approve and merge for now (looks fine)
and then we can arrange a time to go over it together?

gegoune · 2021-07-27T08:40:59Z

Ah, perfect, that's very nice of you, thanks!

gegoune mentioned this pull request Jul 21, 2021

nvim-comment shows lags when working with ts-context-commentstring #26

Closed

terrortylor reviewed Jul 23, 2021

View reviewed changes

terrortylor merged commit 0aa4cbf into terrortylor:main Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hook functionality, update docs #23

Add hook functionality, update docs #23

gegoune commented Jul 10, 2021 •

edited

Loading

gegoune commented Jul 10, 2021

terrortylor commented Jul 23, 2021 •

edited

Loading

terrortylor Jul 23, 2021

terrortylor Jul 25, 2021

gegoune Jul 25, 2021

terrortylor Jul 25, 2021

terrortylor Jul 23, 2021

gegoune Jul 24, 2021

terrortylor Jul 25, 2021

terrortylor Jul 23, 2021

terrortylor Jul 23, 2021

gegoune Jul 24, 2021

terrortylor Jul 25, 2021

terrortylor Jul 23, 2021

gegoune Jul 24, 2021

gegoune Jul 24, 2021

terrortylor Jul 25, 2021

terrortylor Jul 23, 2021

gegoune Jul 24, 2021

gegoune Jul 24, 2021

terrortylor Jul 25, 2021

terrortylor Jul 23, 2021

gegoune Jul 24, 2021

terrortylor Jul 23, 2021

terrortylor Jul 25, 2021

gegoune Jul 25, 2021

mvllow commented Jul 23, 2021

gegoune commented Jul 25, 2021

terrortylor commented Jul 27, 2021

gegoune commented Jul 27, 2021

Add hook functionality, update docs #23

Add hook functionality, update docs #23

Conversation

gegoune commented Jul 10, 2021 • edited Loading

gegoune commented Jul 10, 2021

terrortylor commented Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvllow commented Jul 23, 2021

gegoune commented Jul 25, 2021

terrortylor commented Jul 27, 2021

gegoune commented Jul 27, 2021

gegoune commented Jul 10, 2021 •

edited

Loading

terrortylor commented Jul 23, 2021 •

edited

Loading