[comfysage]

fix #1018

additional changes:

• feat: enable auto_integrations by default
• fix(integrations): options can be nil
• fix(integrations): use O over require("catppuccin").options
• fix(colorful_winsep): color option can be invalid
• fix(gitsigns): option logic
• fix(indent_blankline): do not index table with nil
• fix(mini): color option can be invalid
• fix(navic): do not use CtpColor for color fields
• fix(snacks): color option can be invalid
• fix(telescope): make integration options a boolean this was a table even though it didnt have any options

[vollowx]

That's a huge overhaul!

About the default options, I think it'd be fine to keep them, just removing the enabled field. It would be more complicated and harder to maintain if the default values are set all over the place.

[comfysage]

i agree about keeping the default options inside the table but i think the integrations should still handle nil values better. handling nil values is also necessary to prevent lsp warnings.

[vollowx]

i agree about keeping the default options inside the table but i think the integrations should still handle nil values better. handling nil values is also necessary to prevent lsp warnings.

Do you mean LSP warnings inside this project or at the user config? Inside this project we can safely assume that directly accessing such field will not result in nil, so perhaps modifying the type declarations helps. I cannot verify either of them right now so I'm not sure about the user side one.

[comfysage]

Do you mean LSP warnings inside this project or at the user config? Inside this project we can safely assume that directly accessing such field will not result in nil, so perhaps modifying the type declarations helps. I cannot verify either of them so I'm not sure about the user side one.

the config options should stay nillable to avoid diagnostic warnings in user configs, so we cant change the type definitions. checking for nil is often really simple and in my mind required to avoid bugs. additionally, it is necessary to check for nil so that the lsp can check if the actual type of the option is used correctly in the integration.

sorry for the giant wall of text

[vollowx]

the config options should stay nillable to avoid diagnostic warnings in user configs, so we cant change the type definitions. checking for nil is often really simple and in my mind required to avoid bugs. additionally, it is necessary to check for nil so that the lsp can check if the actual type of the option is used correctly in the integration.

sorry for the giant wall of text

That's reasonable I think.

Originally I thought that falling back on nil checks might let developers ignore some initializing problems, and not falling back makes sure that you get an error when providing neither a default value nor a user config value.

But since the LSP is not recognizing the types from Neovim table merging, it might be better to handle nils for better LSP type checking, as you said.

When do you think shall we merge this? After adding nil checking or leave it in another PR?

[comfysage]

i already added nil checking in this pr:)

[vollowx]

I mean for those that are not colors, like those in barbecue and blink cmp. But it'd be fine not to do so now since the default values are present.

I think we can merge now?

[comfysage]

I mean for those that are not colors, like those in barbecue and blink cmp. But it'd be fine not to do so now since the default values are present.

these dont really need type checking since theyre either bools (and nil is falsy) or enum strings (in which case theyre always compared to a string anyways). so for all of these cases the lsp is totally able to parse the types.

i can still add explicit nil checking in a separate pr if youd like?

[vollowx]

I think we don't need to since the LSP works on this and default values are passed, so it'd all good