feat: relay on languagetree commentstring as much as possible #86

mortezadadgar · 2023-11-20T13:06:00Z

The idea is taken from mini.comment but it's a little simpler as i don't why languagetree children need to be traversed as well:
https://github.com/echasnovski/mini.comment/blob/main/lua/mini/comment.lua#L359

after this change merged i think many languages config can be removed as they're obtained from languagetree there are exceptions like html and jsx that need to stayed for now.

mortezadadgar · 2023-11-20T15:34:16Z

moved the logic to a separate function and removed preconfigured commentstrings not sure rescript, twing and graphql need to be removed as well
the code is mostly is taken from mini.comment i will see if i can merge it into check_node() but it seems to be so complicated

JoosepAlviste · 2023-11-20T17:44:52Z

This looks like a great idea! I'd bring out a couple things for now (as this PR is still a draft):

The filetype commentstring checking logic probably doesn't need to be inside check_node as there we're looping through all the nodes inside a language tree. The filetype commentstring detection is relevant for language trees rather than nodes. I'm not sure what the best place to put the logic would be, though.
One problem with the current location is that if the cursor is in a css block inside tsx, for example, then get_node_at_cursor_start_of_line would return the tsx language tree, rather than the css one, which would get the commentstring from tsx and wouldn't trigger your logic. A visual example:

const MyStyledComponent = styled`
  |color: red;
  ^ cursor is here
`

where get_node_at_cursor_start_of_line would check the current node (css), see that it's not in the languages config, check it's parent (tsx), see that it IS in the config, and use that for the next calculations. Hopefully that made sense 😅

Removing the configs for languages with multiline comments (e.g., lua) would mean that the advanced commenting integration with Comment.nvim wouldn't be able to do multiline comments. I would still keep those in the configuration.

mortezadadgar · 2023-11-20T17:56:24Z

thanks for suggestions I will take a look but i think we should take away removing commentstring from config for now it seems to be tricky

echasnovski · 2023-11-20T19:09:29Z

The idea is taken from mini.comment but it's a little simpler as i don't why languagetree children need to be traversed as well: https://github.com/echasnovski/mini.comment/blob/main/lua/mini/comment.lua#L359

The way it works in 'mini.comment' is by traversing all possible trees in the buffer but checking only those that contain reference position. It also makes sure that the "most specific" child provides the 'commentstring' (hence the check for strictly increased level of nesting). I think that in theory there can be complex situations in which a proper source for 'commentstring' is chosen. For example, if there is a language tree A (providing its own 'commentstring') with children AA and AB (also with own values), and AA has also a children AAA (with fifth value). Which value of 'commentstring' should be chosen? 'mini.comment' goes for the language tree with highest nesting (first among those ones, to be precise).

echasnovski · 2023-11-21T15:42:54Z

lua/ts_context_commentstring/internal.lua

  return M.check_node(node, language_config, key)
 end

+function M.check_ts_node(language_tree)
+  local cs = vim.filetype.get_option(language_tree:lang(), 'commentstring')


Language of a tree doesn't always equal to the filetype it is usually used. See vim.treesitter.language.get_filetypes() (beware that it is added only in Neovim 0.9.

yeah what i'm doing already is to obtain the language under the cursor and skip the comment type and seems to work fine(missed that part in new implementation)

mortezadadgar · 2023-11-22T06:54:23Z

@JoosepAlviste this pr is almost done i just don't which languages can be removed from the config without breaking anything
one idea is to leave it as it's but that would make this pr useless

JoosepAlviste

This looks pretty good already! I haven't tested it too much yet, but had a few comments.

One thing to add is the docs, we should say that setting the commentstring in an ftplugin (or FileType autocmd) should be the first option to consider as that would work with the general Vim ecosystem much better. For fancier behavior, users can configure this plugin directly (with treesitter node specific commentstrings and separate multiline commentstrings from single line ones).

JoosepAlviste · 2023-11-26T09:45:15Z

lua/ts_context_commentstring/internal.lua

@@ -51,10 +51,8 @@ end
 function M.calculate_commentstring(args)
  args = args or {}
  local key = args.key or '__default'
-  local location = args.location or nil


We will need to keep allowing to pass in a custom location as this is required for the Comment.nvim integration. The commentstring shouldn't always be calculated from the cursor line's start (e.g., when using motions like gciw)

JoosepAlviste · 2023-11-26T10:35:24Z

lua/ts_context_commentstring/internal.lua

  return M.check_node(node, language_config, key)
 end

+function M.check_ts_node(language_tree)


The name for this isn't very indicative of what the function does, I'd rename it to something like "get commentstring from language tree".

Also, it would be nice to have type hints for the parameter and return type.

JoosepAlviste · 2023-11-26T10:36:50Z

lua/ts_context_commentstring/internal.lua

  return M.check_node(node, language_config, key)
 end

+function M.check_ts_node(language_tree)
+  local cs = vim.filetype.get_option(language_tree:lang(), 'commentstring')


Even if this variable is used in a very small scope, I think that it still makes sense to avoid abbreviations in variable names. "cs" makes me think of Counter Strike 😄

Same thing above

JoosepAlviste · 2023-11-26T14:46:58Z

lua/ts_context_commentstring/config.lua

-    astro = '<!-- %s -->',
-    c = { __default = '// %s', __multiline = '/* %s */' },
-    css = '/* %s */',
-    glimmer = '{{! %s }}',
-    graphql = '# %s',
-    handlebars = '{{! %s }}',
-    html = '<!-- %s -->',
-    lua = { __default = '-- %s', __multiline = '--[[ %s ]]' },
-    nix = { __default = '# %s', __multiline = '/* %s */' },
-    php = { __default = '// %s', __multiline = '/* %s */' },
-    python = { __default = '# %s', __multiline = '""" %s """' },
    rescript = { __default = '// %s', __multiline = '/* %s */' },
-    scss = { __default = '// %s', __multiline = '/* %s */' },
-    sh = '# %s',
-    bash = '# %s',
    solidity = { __default = '// %s', __multiline = '/* %s */' },
-    sql = '-- %s',
    svelte = '<!-- %s -->',
    twig = '{# %s #}',
-    typescript = { __default = '// %s', __multiline = '/* %s */' },
-    vim = '" %s',
    vue = '<!-- %s -->',
-    zsh = '# %s',


I'd keep the language configs that:

Do not have a default in Neovim core

Have an extra multiline comment style config

as removing those would result in breaking changes and would require people to change their configs after updating this plugin. I would very much like to avoid any breaking changes, we could group those together with other breaking changes in the future (e.g., removing the config option for good).

Here are the language configs I would keep:

astro - no default commentstring

c - has a multilline config

grimmer - no default

graphql - no default

handlebars - no default

lua - multiline config

nix - multiline config

php - multiline config

python - multiline config

rescript - multiline config

scss - multiline config

solidity - multiline config

sql - no default

svelte - no default

twig - no default

typescript - multiline config

vue - no default

The others can be deleted, I think.

This at least makes it so that new languages with a single commenting style would work out of the box, and wouldn't need to be configured in this plugin.

We can figure out how to get rid of the multiline comment configs in the future (maybe using :h comments somehow?).

JoosepAlviste · 2023-11-26T14:57:21Z

lua/ts_context_commentstring/utils.lua

-    if ltree:contains(range) and vim.tbl_contains(only_languages, ltree:lang()) then
+    -- always exclude the comment language as it's returning invalid
+    -- commentstring
+    if ltree:contains(range) and ltree:lang() ~= 'comment' then


There's also jsdoc, phpdoc, and probably other parsers as well which don't map directly to a filetype. Do we need to handle those too?

mortezadadgar mentioned this pull request Nov 20, 2023

Incorrent commentstring for inline markdown code blocks #83

Closed

mortezadadgar marked this pull request as draft November 20, 2023 13:08

feat: relay on languagetree commentstring as much as possible

5bece0f

mortezadadgar force-pushed the patch-3 branch from 2e383f4 to 21abb6f Compare November 20, 2023 15:32

remove some preconfigured commentstring

0d34afd

mortezadadgar force-pushed the patch-3 branch from 21abb6f to 0d34afd Compare November 20, 2023 15:32

mortezadadgar mentioned this pull request Nov 20, 2023

always use line-wise C commentstring #85

Merged

echasnovski reviewed Nov 21, 2023

View reviewed changes

mortezadadgar force-pushed the patch-3 branch 2 times, most recently from 53f488f to 9d326f3 Compare November 21, 2023 20:06

new implementation

40a0b06

mortezadadgar force-pushed the patch-3 branch from 9d326f3 to 40a0b06 Compare November 22, 2023 06:56

JoosepAlviste requested changes Nov 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: relay on languagetree commentstring as much as possible #86

feat: relay on languagetree commentstring as much as possible #86

mortezadadgar commented Nov 20, 2023 •

edited

Loading

mortezadadgar commented Nov 20, 2023 •

edited

Loading

JoosepAlviste commented Nov 20, 2023

mortezadadgar commented Nov 20, 2023

echasnovski commented Nov 20, 2023

echasnovski Nov 21, 2023

mortezadadgar Nov 21, 2023 •

edited

Loading

mortezadadgar commented Nov 22, 2023

JoosepAlviste left a comment

JoosepAlviste Nov 26, 2023

JoosepAlviste Nov 26, 2023

JoosepAlviste Nov 26, 2023

JoosepAlviste Nov 26, 2023

JoosepAlviste Nov 26, 2023

feat: relay on languagetree commentstring as much as possible #86

Are you sure you want to change the base?

feat: relay on languagetree commentstring as much as possible #86

Conversation

mortezadadgar commented Nov 20, 2023 • edited Loading

mortezadadgar commented Nov 20, 2023 • edited Loading

JoosepAlviste commented Nov 20, 2023

mortezadadgar commented Nov 20, 2023

echasnovski commented Nov 20, 2023

echasnovski Nov 21, 2023

Choose a reason for hiding this comment

mortezadadgar Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

mortezadadgar commented Nov 22, 2023

JoosepAlviste left a comment

Choose a reason for hiding this comment

JoosepAlviste Nov 26, 2023

Choose a reason for hiding this comment

JoosepAlviste Nov 26, 2023

Choose a reason for hiding this comment

JoosepAlviste Nov 26, 2023

Choose a reason for hiding this comment

JoosepAlviste Nov 26, 2023

Choose a reason for hiding this comment

JoosepAlviste Nov 26, 2023

Choose a reason for hiding this comment

mortezadadgar commented Nov 20, 2023 •

edited

Loading

mortezadadgar commented Nov 20, 2023 •

edited

Loading

mortezadadgar Nov 21, 2023 •

edited

Loading