p00f / nvim-ts-rainbow

Rainbow parentheses for neovim using tree-sitter. Use https://sr.ht/~p00f/nvim-ts-rainbow instead
Apache License 2.0
869 stars 67 forks source link

fix: plugin not work when colors are more than termcolors #137

Closed adoyle-h closed 2 years ago

adoyle-h commented 2 years ago

If you defined {colors = { "#cc241d", "#a89984" }, termcolors = {}}, you can't reproduce the issue. Because there are seven colors and termcolors in default config.

If you defined colors more than seven,

colors = { "#cc241d", "#a89984", "#b16286", "#d79921", "#689d6a", "#d65d0e", "#458588", "#005f87" },
termcolors = {},

Or colors' length more than termcolors',

colors = { "#cc241d", "#a89984"},
termcolors = { 3 },

then the issue can be reproduced.

close #120

adoyle-h commented 2 years ago

@p00f Would you please review this PR?

p00f commented 2 years ago

You haven't addressed the comments from the first review yet

adoyle-h commented 2 years ago

You haven't addressed the comments from the first review yet

I haven't seen any comments. Where is it?

p00f commented 2 years ago

https://github.com/p00f/nvim-ts-rainbow/pull/137#discussion_r965649140

adoyle-h commented 2 years ago

#137 (comment)

Sorry, it only shows part of the comments when mouse hover. Click this link but nothing shows up. Can you post a screenshot of first review comments?

p00f commented 2 years ago

image

adoyle-h commented 2 years ago

@p00f Thanks. The difference is that highlight default rainbowcol guifg=#FFFFFF ctermfg=nil will throw error when termcolors[i] is nil.

No need complex termcolors[(i % termcolors == 0) and #termcolors or (i % termcolors)]. Just use vim.api.nvim_set_hl, it handles the nil values.

p00f commented 2 years ago

It "handles" nil values by doing nothing, which is not what I want. When something is wrong, an error is better than silently doing the wrong thing

p00f commented 2 years ago

https://github.com/p00f/nvim-ts-rainbow/commit/a48d442d47e2760fdc63f00e8edf470c51548ae9

adoyle-h commented 2 years ago

@p00f It still has problem when user set config like that,

{
    colors = {
        '#005f87', '#d75f00', '#87ff5f',
    },
    termcolors = {},
}

termcolors[(i % #termcolors == 0) and #termcolors or (i % #termcolors)] is nil. When user run nvim in gui, he may set termcolors = {}.

When something is wrong, an error is better than silently doing the wrong thing

It's hard to debug the error for user. The error message shows loop or previous error loading module 'rainbow.internal' which is no relation about termcolors = {}. User will be confused.

p00f commented 2 years ago

https://github.com/p00f/nvim-ts-rainbow/commit/fad8badcd9baa4deb2cf2a5376ab412a1ba41797

This should be fixed now

It's hard to debug the error for user. The error message shows loop or previous error loading module 'rainbow.internal' which is no relation about termcolors = {}. User will be confused.

The user can open an issue