Skip to content
This repository has been archived by the owner on Jan 2, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

adoyle-h
Copy link

@adoyle-h adoyle-h commented Sep 8, 2022

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
Copy link
Author

@p00f Would you please review this PR?

@adoyle-h
fix: plugin not work when colors are more than termcolors
5123156 
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 p00f#120
@p00f
Copy link
Owner

p00f commented Sep 14, 2022 •
edited

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

@adoyle-h
Copy link
Author

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

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

@p00f
Copy link
Owner

p00f commented Sep 15, 2022

#137 (comment)

@adoyle-h
Copy link
Author

#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
Copy link
Owner

p00f commented Sep 15, 2022

image

@adoyle-h
Copy link
Author

@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
Copy link
Owner

p00f commented Sep 15, 2022 •
edited

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 p00f closed this in a48d442 Sep 15, 2022
@p00f
Copy link
Owner

p00f commented Sep 15, 2022

a48d442

@adoyle-h
Copy link
Author

@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
Copy link
Owner

p00f commented Sep 17, 2022

fad8bad

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't get the plugin to work
2 participants
@adoyle-h @p00f