Abort visual commenting if it makes no sense #199

RaafatTurki · 2022-08-03T17:00:52Z

RaafatTurki
Aug 3, 2022

Hey there, awesome plugin BTW!
This might not be strictly Comment.nvim specific but I'd love a pointer in the right direction.

This issue is about nested filetype support so as the readme says I'm using nvim-ts-context-commentstring which integrates great with the readme pre_hook snippet

Consider the following example

1   <html>
2     <body>
3       <script>
4       let name = "raafat"
5       function print_name(name) {
6         console.log(name)
7       }
8       </script>
9     </body>
10  </html>

Visually selecting lines 1 through 5 and commenting produces an unwanted mess:

<!-- <html> -->
<!--   <body> -->
<!--     <script> -->
<!--     let name = "Raafat" -->
<!--     function print_name(name) { -->
      console.log(name)
    }
    </script>
  </body>
</html>

My aim is to calculate the commentstring at the start and end of the visual selection and only comment if they match.
I've found out that returning %s in the pre_hook results in "aborting" (is there a proper way to do this?).
Let me know what you think.

numToStr · 2022-08-04T08:08:47Z

numToStr
Aug 4, 2022
Maintainer

Very interesting use case. I tried and the following seems to work

require('Comment').setup({
    pre_hook = function(ctx)
        local start_range = {
            srow = ctx.range.srow,
            erow = ctx.range.srow,
            scol = ctx.range.scol,
            ecol = ctx.range.scol,
        }

        local end_range = {
            srow = ctx.range.erow,
            erow = ctx.range.erow,
            scol = ctx.range.ecol,
            ecol = ctx.range.ecol,
        }

        ctx.range = start_range
        local start_cstr = require('Comment.ft').calculate(ctx)

        ctx.range = end_range
        local end_cstr = require('Comment.ft').calculate(ctx)

        return start_cstr == end_cstr and start_cstr or ''
    end,
})

FYI, you don't need https://github.com/JoosepAlviste/nvim-ts-context-commentstring unless you need jsx (which might also get integrated in future #133)

0 replies

RaafatTurki · 2022-08-05T17:45:58Z

RaafatTurki
Aug 5, 2022
Author

Great snippet, it works nicely!

The reaon I use nvim_ts_ctx_cs is not jsx, it's embedded js in html:
Here's a minimal testing snippet

<html>
  <body>
    <script>
      let name = "raafat"
    </script>
  </body>
</html>

Which after a while of linewise commenting turns into:

<html>
  <body>
    <script>
      <!-- // <!-- // <!-- // <!-- // let name = "raafat" --> --> --> -->
    </script>
  </body>
</html>

The same happens with js/ts in svelte files, Is this a known issue or should I debug my nvim setup?

0 replies

numToStr · 2022-08-05T17:49:22Z

numToStr
Aug 5, 2022
Maintainer

You must be hitting this issue #62 (comment), which will, sadly, always remain a limitation

0 replies

RaafatTurki · 2022-08-05T18:12:55Z

RaafatTurki
Aug 5, 2022
Author

I see.
How do you reckon a nvim_ts_ctx_cs based version of the snippet you've provided would look like?

0 replies

numToStr · 2022-08-06T13:33:54Z

numToStr
Aug 6, 2022
Maintainer

Hmmmm I am not sure as I don't use ts-context-commenstring. Maybe ask the plugin author for help. I guess it would be pretty similar to what I suggested earlier; calculate the commenstring twice, first for the starting range and other for the ending range then compare them.

0 replies

RaafatTurki · 2022-08-09T00:44:57Z

RaafatTurki
Aug 9, 2022
Author

I've asked the plugin author for a get_visual_end_location so I can compare values and they've delivered

I ended up with this pre_hook

pre_hook = function(ctx)
  local comment_utils = require 'Comment.utils'
  local tsctxcs_utils = require 'ts_context_commentstring.utils'
  local tsctxcs_internal = require 'ts_context_commentstring.internal'
  
  local comment_strings = {}
  local cs_type = ctx.ctype == comment_utils.ctype.line and '__default' or '__multiline'

  local calc_cs = function (posision)
    return tsctxcs_internal.calculate_commentstring({
      key = cs_type,
      location = posision
    })
  end

  local abort_cs = function (msg)
    vim.notify(msg)
    return '%s'
  end

  -- visual commenting
  if ctx.cmotion == comment_utils.cmotion.v or ctx.cmotion == comment_utils.cmotion.V then
    table.insert(comment_strings, calc_cs(tsctxcs_utils.get_visual_start_location()))
    table.insert(comment_strings, calc_cs(tsctxcs_utils.get_visual_end_location()))
  -- block commenting
  elseif ctx.ctype == comment_utils.ctype.block then
    table.insert(comment_strings, calc_cs(tsctxcs_utils.get_cursor_location()))
  -- linewise commenting
  else
    table.insert(comment_strings, calc_cs(nil))
  end

  -- compare comment_strings and return one
  local last_cs = nil
  for i, cs in ipairs(comment_strings) do
    if i > 1 and cs ~= last_cs then
      return abort_cs('Commenting aborted due to mismatching comment strings')
    end
    last_cs = cs
  end

  return last_cs
end

It works great however:

I can't abort the way you did by returning '' (errors "invalid commentstring") hence I return '%s'
Aborting results in inserting a "change" that does nothing into the undo history which is inconvenient

Both of those issues are a result of not having an aborting mechanism in the pre_hook.
I suggest changing the pre_hook signature to function(ctx, abort) where abort is a callable function that blocks any code execution that comes after pre_hook.

Let me know what you think.

4 replies

numToStr Aug 9, 2022
Maintainer

I can't abort the way you did by returning ''

You sure can both '' and %s are invalid

I suggest changing the pre_hook signature to function(ctx, abort) where abort is a callable function that blocks any code execution that comes after pre_hook.

I think abort argument is redundant, because commenstring parsing happens immediately after pre_hook and the plugin panics if the returned string is invalid. Before any changes to the buffer.

Aborting results in inserting a "change" that does nothing into the undo history which is inconvenient
Both of those issues are a result of not having an aborting mechanism in the pre_hook.

Might be because of how you are aborting? I am not sure. Can you check if the issue still exists with my snippet?

RaafatTurki Aug 9, 2022
Author

You sure can both '' and %s are invalid

I think you've meant "valid".
Either way that's not the case, as '' throws this error.

... because commenstring parsing happens immediately after pre_hook and the plugin panics if the returned string is invalid ...

We could change that behavior, I'm thinking an internal is_aborting boolean that defaults to false where the abort() function sets to true (and any other internal aborting features you might wanna add) and any commenstring parsing would not happen since it checks for is_aborting, aborting is an actual outcome that should be considered seriously, passing '%s' is a workaround.
I would even PR this mechanism if it gets merged.

... Can you check if the issue still exists with my snippet?

The same issue occurs, I'll again note that an aborting mechanism would solve this.

numToStr Aug 9, 2022
Maintainer

I think you've meant "valid".

Ohh yeah, you are right. %s is valid, in this case both RHS and LHS will be '' (empty string) and will proceed to updating the buffer, hence the issue of undo history which is expected.

We could change that behavior, I'm thinking an internal is_aborting boolean that defaults to false where the abort() function sets to true (and any other internal aborting features you might wanna add)

Sorry, this is not convincing for me. Adding stuff for a specific use case is a no no for me, unless couple of people wanted this.

RaafatTurki Aug 9, 2022
Author

It's alright, I'm sorry this didn't work out.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort visual commenting if it makes no sense #199

{{title}}

Replies: 6 comments 4 replies

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Abort visual commenting if it makes no sense #199

RaafatTurki Aug 3, 2022

Replies: 6 comments · 4 replies

numToStr Aug 4, 2022 Maintainer

RaafatTurki Aug 5, 2022 Author

numToStr Aug 5, 2022 Maintainer

RaafatTurki Aug 5, 2022 Author

numToStr Aug 6, 2022 Maintainer

RaafatTurki Aug 9, 2022 Author

numToStr Aug 9, 2022 Maintainer

RaafatTurki Aug 9, 2022 Author

numToStr Aug 9, 2022 Maintainer

RaafatTurki Aug 9, 2022 Author

RaafatTurki
Aug 3, 2022

Replies: 6 comments 4 replies

numToStr
Aug 4, 2022
Maintainer

RaafatTurki
Aug 5, 2022
Author

numToStr
Aug 5, 2022
Maintainer

RaafatTurki
Aug 5, 2022
Author

numToStr
Aug 6, 2022
Maintainer

RaafatTurki
Aug 9, 2022
Author

numToStr Aug 9, 2022
Maintainer

RaafatTurki Aug 9, 2022
Author

numToStr Aug 9, 2022
Maintainer

RaafatTurki Aug 9, 2022
Author