feat: Auto calculate `commentstring` #62

tjdevries · 2021-11-02T15:31:51Z

Not all the way done, but figured I'd shoot this over to you so you can take a look and let me know what you think.

numToStr · 2021-11-02T15:40:45Z

Thanks a ton. I'll have a look at this on the weekend or so.

tjdevries · 2021-11-03T00:14:10Z

Should I be doing:

            -- TODO: Dunno if this is any good or not.
            return config[ctx.ctype] or config[1]

Or just return the config[ctx.ctype] and if it doesn't have block comment, then it's all done?

numToStr · 2021-11-03T05:16:58Z

Or just return the config[ctx.ctype] and if it doesn't have a block comment, then it's all done?

I thought about this before and IMO we should throw an error if the block comment isn't found when doing gb and friends. Although this should be done in its separate PR.

One question, Is adding context awareness by default a good idea? I mean some (like me) don't use context-commentstring and for some language like rust, this is not necessarily useful to calculate comment string.

tjdevries · 2021-11-03T23:21:38Z

I can make it short circuit for languages that don't have additional configuration.

My thought would be is if you know this is definitely something people will use and you can make it in a way that doesn't require any additional configuration, why not just enable it? It might cost a few bonus cpu cycles, but you obviously can afford it. You compile rust code 😂

numToStr · 2021-11-04T04:40:22Z

It might cost a few bonus cpu cycles, but you obviously can afford it

Yeah I guess we can do that 😄

But enter jsx. And this is one of the most bizarre things in this world. This literally deserves its own plugin.

// When only attribute is on the line, then `//%s` is used.
export function Element() {
  return (
    <div>
      <h1
        // id="foo"
        // class="lsp"
        data-telescope="good"
      >
        Hello, world!
      </h1>
    </div>
  );
}

// When attribute is on the same line as element, then `/*%s*/` is used.
export function Element() {
  return (
    <div>
      <h1 /* id="foo" class="lsp" */ data-telescope="good" >
        Hello, world!
      </h1>
    </div>
  );
}

// For element, then `{/*%s*/}` is used.
export function Element() {
  return (
    <div>
      {/*<h1 id="foo" class="lsp" data-telescope="good" >*/}
        Hello, world!
      </h1>
    </div>
  );
}

What do you think? Should we try to cover all these cases or let the nvim-ts-context-commenstring handle all this?

sQVe · 2021-11-04T06:02:12Z

I just wanted to leave my 0.02$ and voice that I think the marriage between Comment.nvim and nvim-ts-context-commentstring works really well - especially now with the new changes in type and location.

tjdevries · 2021-11-04T22:46:28Z

What changes do you mean for type and location? I'm not familiar.

What do you think? Should we try to cover all these cases or let the nvim-ts-context-commenstring handle all this?

I don't think you have to handle everything, but providing some framework to make things work reasonably well out of the box in what is essentially 70ish lines of code (the other bits are docs, config updates or renames effectively) seems like not a bad tradeoff. Of course, it's not my plugin :) so you can reject any time you want. I was just thinking this works surprisingly well with no dependencies (it only uses things from vim.treesitter, no deps otherwise).

Another thing is this calculates based on where your cursor currently is, so you can do interesting stuff like if you have multiple filetypes on one line, it will calculate the correct one, but you just get that for free basically from just checking the current node and walking up til you find either an interesting node or the root.

tjdevries · 2021-11-04T22:51:36Z

(Also, people can just do whatever crazy jsx config they want to make happen using ft.set('jsx', { ... }), and/or just using context comment string for that)

numToStr · 2021-11-05T07:12:20Z

What changes do you mean for type and location? I'm not familiar.

He was talking about this JoosepAlviste/nvim-ts-context-commentstring#28 change.

I was just thinking this works surprisingly well with no dependencies (it only uses things from vim.treesitter, no deps otherwise)

The initial code that you showed on your stream, yeah that was also a total surprise to me as well. For something like vue or lua + ffi, this is all that you might need.

But IMO we should hold off adding this now. We can revisit this later :)

tjdevries · 2021-11-09T16:18:44Z

@numToStr if there are changes you want to make, just push directly to this branch and then merge if you want

arpangreat · 2021-11-11T15:25:02Z

So when this pr is going to be merged ? I loving this plugin so far but this will be killer

numToStr · 2021-11-11T15:59:00Z

I am little caught up with my work right now. It could take a while before I merge this :)

arpangreat · 2021-11-11T16:07:30Z

I am little caught up with my work right now. It could take a while before I merge this :)

Fine :) , BTW Great Work man ❤️

tjdevries · 2021-11-13T17:11:01Z

Anything I can do to help it get merged? I want to merge this before I make my TakeTuesday video about it :) Let me know if there is anything missing still for it.

numToStr · 2021-11-13T17:13:23Z

@tjdevries I am probably gonna merge it tomorrow morning :)

tjdevries · 2021-11-14T00:22:11Z

<3 excellent. Thanks. Maybe I will try and sneak in a recording session before this tuesday then :) thanks

lua/Comment/ts.lua

numToStr · 2021-11-14T08:07:20Z

I've done the following changes

Added vue and css commentstring to please the vue devs.
Replaced typescriptreact with tsx. It seems treesitter returns tsx as a language for typescriptreact filetypes.
Removed javascriptreact commenstring. In this case treesitter returns javascript even for javascriptreact filetypes

lua/Comment/ts.lua

tjdevries · 2021-11-17T01:36:58Z

OK, I think I pushed. Checked a bunch of different combos in that file. Feel free to squash and merge or whatever you want to do w/ this after. Excited for it :) working on the video now too

numToStr · 2021-11-17T07:48:17Z

Thanks a ton ❤️. But, I am a little skeptical about this feature as I am pretty sure there will be some potential issue that we haven't found yet (some I did find). IMO 90% of the issues will always come from jsx/tsx trying to be cool. So, what we could do to support auto calculate? I am suggesting to only support proper/full-fledged languages that are injected/embedded something like HTML with <script/> tag for javascript and <style/> for CSS or Lua heredoc inside vimL. This, I guess, would cover most cases with injected languages.

But what about react/jsx gang? Ok, for that we could easily say that you should use nvim-ts-context-commentstring as the builtin integration sucks for jsx. I would also like to discuss with the author to improve the integration story b/w mine and other plugins so that every user doesn't have to copy/paste 10-15 lines to get it working.

only support proper/full-fledged languages that are injected/embedded

To do so I came with this (thanks to ts-playground)

function ft.calculate(ctype)
    local buf = vim.api.nvim_get_current_buf()
    local langtree = vim.treesitter.get_parser(buf)

    local win = vim.api.nvim_get_current_win()
    local row, col = unpack(vim.api.nvim_win_get_cursor(win))
    local range = { row - 1, col, row - 1, col }

    local found
    langtree:for_each_child(function(tree)
        if not found and tree:contains(range) then
            local lang = tree:lang()
            local cstr = ft.get(lang, ctype)
            if cstr then
                found = cstr
            end
        end
    end)

    return found or ft.get(vim.bo.filetype, ctype)
end

This is easy to maintain and reason about. (I am being selfish here 😅). And this is how this works with Lua (ffi), HTML and vue filetype.

2021-11-17.13-00-12.mp4

@tjdevries What's your opinion on this? I need your green light on this :)

tjdevries · 2021-11-17T13:55:09Z

Ah, so main difference is just that instead of being able to do for particular nodes, you'll only do it for embedded languages?

numToStr · 2021-11-17T14:42:22Z

Ah, so main difference is just that instead of being able to do for particular nodes, you'll only do it for embedded languages?

Exactly, only supporting proper languages that are embedded inside one another. No walking nodes or something. No jsx either.

seblj · 2021-11-18T10:36:59Z

It still includes the opening tag. But I am pretty sure that this is just how it works, since I can recreate the behaviour with vit with a clean neovim with no plugins. (It's also this behaviour with vim). However, it fixes the problem where you comment, and them uncomment really fast like I mentioned in a previous comment

numToStr · 2021-11-18T10:45:27Z

It still includes the opening tag. But I am pretty sure that this is just how it works, since I can recreate the behaviour with vit with a clean neovim with no plugins

Yup It does. If you have a whitespace after the opening tag.

seblj · 2021-11-18T10:51:08Z

Yup It does. If you have a whitespace after the opening tag.

That's the thing, I don't have a whitespace. If you could try with this, and do a vit (or gcit to comment, you should see that it includes the starting tag even if you don't have a trailing whitespace) inside one of the tags that span over multiple lines. For example the style or script tags

<template>
  <div>
    <p>Something</p>
  <div>
</template>

<script lang="ts">

import { defineComponent } from 'vue';

export default defineComponent({
  name: 'test',
    
  },
});
</script>

<style>
body {
  height: 100vh;
}
</style>

seblj · 2021-11-18T10:59:49Z

But this isn't a big deal since it is the same behaviour from other comment-plugins like for example vim-commentary. So this isn't something that should be "fixed" here IMO. If there is a fix needed, it should probably be to neovim

numToStr · 2021-11-18T11:05:04Z

@seblj ~~I think something weird is going on with your nvim. Mine is working flawlessly.~~

2021-11-18.16-30-55.mp4

EDIT: ~~(I have targets.vim installed, maybe that's why I am not getting that issue, IDK)~~ Yes, It was targets.vim that fixed the issue for me, I didn't know that 😄. I removed it and I got the same behavior as yours. And yeah, you are right this needs to be fixed in vim/neovim.

seblj · 2021-11-18T11:13:41Z

EDIT: ~~(I have targets.vim installed, maybe that's why I am not getting that issue, IDK)~~ Yes, It was targets.vim that fixed the issue for me, I didn't know that 😄. I removed it and I got the same behavior as yours. And yeah, you are right this needs to be fixed in vim/neovim.

Oh okay, that's good to know! I might check out targets.vim later then. Glad we figured that out😄

numToStr · 2021-11-18T11:36:51Z

I have now refactored the range thingy. Apart from cleanup, I think everything is good to go. @seblj Care to test (and don't forget to install targets.vim 😆)?

PS: I might need to do some refactoring after this PR is merged.

seblj · 2021-11-18T11:44:22Z

I have now refactored the range thingy. Apart from cleanup, I think everything is good to go. @seblj Care to test (and don't forget to install targets.vim 😆)?

There seems to be some issues when there are whitespaces in the tag-block. (I installed targets.vim).

Screen.Recording.2021-11-18.at.12.41.08.mov

numToStr · 2021-11-18T11:58:22Z

Hmm, I am getting this kinda similar issue in HTML file but vue is fine. I am looking into this issue.

numToStr · 2021-11-18T12:28:22Z

@seblj I get what's happening. Assuming the following style tag, doing gcit will not work.

<style>
    .comment {
^   ^ `css` starts from here
This whitespace area is `html`
        border: 1px solid red;
    }
</style>

(could easily be added to one function that is get_currently_ts_range() or something)

Now I think @tjdevries was right with this one.

numToStr · 2021-11-18T13:23:49Z

@seblj I get what's happening. Assuming the following style tag, doing gcit will not work.
<style>
    .comment {
    ^ `css` starts from here
^ This whitespace area is `html`
        border: 1px solid red;
    }
</style>

Apart from this tag issue (which only happens where one language ends and the other starts), everything else looks good to me. We can still say you can use nvim-ts-context-commentstring if you are not happy with the builtin one 😅.

seblj · 2021-11-18T22:17:34Z

Okay so I found out some of the issues and why they happen (I think). It seems to be the case like you say that treesitter is interpreting it as another language, but I could only reproduce this if the newline is between the starting tag and the first line inside the tags.

<script lang="ts">

import { defineComponent } from 'vue';
export default defineComponent({
  name: 'test',
  },
});
</script>

With the newline there, treesitter interprets that as filetype vue, and I don't think there is much we can do about that. One thing I think we can do is calculate the commentstring based on where the cursor is when doing the motion. Right now it looks like the cursor jumps to the start of the motion, and then when finding the language where the cursor is currently, the language is returned as vue. After this the cursor returns to where it was (probably has something to do with the sticky option). If one is holding the cursor over the newline and uncomment, there isn't much we can do (to my knowledge) since treesitter interprets this as vue.

However there is also a slight issue with empty lines if the commentstring surrounds the string (commentstrings like /* %s */ for example). If there is a newline inside the tag when doing gcit for example, the newline will only get /* inserted on the line. This will mess with the logic of checking if a line is commented or not, so therefore it will try to comment this again instead of uncommenting.

<style>
/* body { */
/*   height: 100vh; */
/*
/* } */
</style>

Note that this will not be an issue with commentstrings like // %s, and also not for those who chose to ignore the newlines in lua pattern option in the setup function. I also don't think this is related to this PR, so this could probably be fixed in another PR.

If we add the rhs of the commentstring (*/) to the newline, after /* the problem is fixed. Like this

<style>
/* body { */
/*   height: 100vh; */
/* */
/* } */
</style>

numToStr · 2021-11-19T07:40:20Z

With the newline there, treesitter interprets that as filetype vue, and I don't think there is much we can do about that.

Yeah, that certainly a limitation. But as I said we can still rely on nvim-ts-context-commenstring to tackle this.

However there is also a slight issue with empty lines if the commentstring surrounds the string (commentstrings like /* %s / for example). If there is a newline inside the tag when doing gcit for example, the newline will only get / inserted on the line. This will mess with the logic of checking if a line is commented or not, so therefore it will try to comment this again instead of uncommenting.

Ahh, yes I also noticed this sometimes. But this a general issue I can fix this later.

Anyways other than these I think we are good to go. I would like to merge this. What do you say?

seblj · 2021-11-19T08:08:44Z

Yeah, that certainly a limitation. But as I said we can still rely on nvim-ts-context-commenstring to tackle this.

That is actually also a problem when using nvim-ts-context-commentstring so I think that issue maybe should be resolved by the treesitter parser of vue or injections in nvim-treesitter😅 (unless that is the intended behaviour🤷‍♂️)

Anyways other than these I think we are good to go. I would like to merge this. What do you say?

Yes, other than those two, it looks to be working great

numToStr · 2021-11-19T08:39:45Z

Noice. I'll just cleanup some stuff and update readme then I'll merge this :)

clason · 2021-11-20T10:26:11Z

Just a heads-up: this PR completely broke comment.nvim for filetypes without a parser: you just get

E5108: Error executing lua ...local/share/nvim/runtime/lua/vim/treesitter/language.lua:25: no parser for 'tex'
 language, see :help treesitter-parsers
stack traceback:
        [C]: in function 'error'
        ...local/share/nvim/runtime/lua/vim/treesitter/language.lua:25: in function 'require_language'
        /usr/local/share/nvim/runtime/lua/vim/treesitter.lua:38: in function '_create_parser'
        /usr/local/share/nvim/runtime/lua/vim/treesitter.lua:93: in function 'get_parser'
        ...m/site/pack/packer/start/comment.nvim/lua/Comment/ft.lua:86: in function 'calculate'
        ...ite/pack/packer/start/comment.nvim/lua/Comment/utils.lua:217: in function 'parse_cstr'
        ...te/pack/packer/start/comment.nvim/lua/Comment/opfunc.lua:63: in function 'opfunc'
        ...e/pack/packer/start/comment.nvim/lua/Comment/comment.lua:166: in function <...e/pack/packer/start/c
omment.nvim/lua/Comment/comment.lua:165>

numToStr · 2021-11-20T10:35:04Z

Fixed 7627727

tjdevries added 3 commits November 9, 2021 08:56

feat: Auto calculate commentstring

6537549

fixup

ed18b8a

fixes from stream

80dd5f1

tjdevries force-pushed the lang branch from 3a2ba4f to 80dd5f1 Compare November 9, 2021 15:14

numToStr reviewed Nov 14, 2021

View reviewed changes

lua/Comment/ts.lua Outdated Show resolved Hide resolved

some fixes

96db471

numToStr reviewed Nov 14, 2021

View reviewed changes

lua/Comment/ts.lua Outdated Show resolved Hide resolved

seblj reviewed Nov 16, 2021

View reviewed changes

lua/Comment/ts.lua Outdated Show resolved Hide resolved

fix: fix off by one

964be1c

refactor range

3412cff

numToStr mentioned this pull request Nov 18, 2021

Feature: Add "duplicate" (yank, comment, paste) function? #70

Closed

pass correct range

68e76c1

numToStr added 2 commits November 18, 2021 18:54

it can be better

30d2a53

cleanup

bc37d4a

numToStr mentioned this pull request Nov 19, 2021

Invalid css comments on empty lines #71

Closed

numToStr added 3 commits November 20, 2021 12:12

readme update

4761d44

fix

7932f57

typo

1394f6a

numToStr merged commit 89e860a into numToStr:master Nov 20, 2021

numToStr mentioned this pull request Jan 25, 2022

feat: add svelte support #107

Merged

numToStr mentioned this pull request May 21, 2022

Commenting out the first line of CSS inside of an html file is broken #164

Closed

numToStr mentioned this pull request Aug 5, 2022

Abort visual commenting if it makes no sense #193

Closed

feat: Auto calculate commentstring #62

feat: Auto calculate commentstring #62

Conversation

tjdevries commented Nov 2, 2021

numToStr commented Nov 2, 2021

tjdevries commented Nov 3, 2021

numToStr commented Nov 3, 2021

tjdevries commented Nov 3, 2021

numToStr commented Nov 4, 2021

sQVe commented Nov 4, 2021

tjdevries commented Nov 4, 2021

tjdevries commented Nov 4, 2021

numToStr commented Nov 5, 2021

tjdevries commented Nov 9, 2021

arpangreat commented Nov 11, 2021

numToStr commented Nov 11, 2021

arpangreat commented Nov 11, 2021

tjdevries commented Nov 13, 2021

numToStr commented Nov 13, 2021

tjdevries commented Nov 14, 2021

numToStr commented Nov 14, 2021

tjdevries commented Nov 17, 2021

numToStr commented Nov 17, 2021

tjdevries commented Nov 17, 2021

numToStr commented Nov 17, 2021 • edited Loading

seblj commented Nov 18, 2021

numToStr commented Nov 18, 2021

seblj commented Nov 18, 2021

seblj commented Nov 18, 2021

numToStr commented Nov 18, 2021 • edited Loading

seblj commented Nov 18, 2021

numToStr commented Nov 18, 2021

seblj commented Nov 18, 2021

numToStr commented Nov 18, 2021 • edited Loading

numToStr commented Nov 18, 2021 • edited Loading

numToStr commented Nov 18, 2021

seblj commented Nov 18, 2021

numToStr commented Nov 19, 2021

seblj commented Nov 19, 2021

numToStr commented Nov 19, 2021

clason commented Nov 20, 2021

numToStr commented Nov 20, 2021

feat: Auto calculate `commentstring` #62

feat: Auto calculate `commentstring` #62

numToStr commented Nov 17, 2021 •

edited

Loading

numToStr commented Nov 18, 2021 •

edited

Loading

numToStr commented Nov 18, 2021 •

edited

Loading

numToStr commented Nov 18, 2021 •

edited

Loading