nixvim icon indicating copy to clipboard operation
nixvim copied to clipboard

fix(dashboard-nvim): update to new setup schema

Open wizardlink opened this issue 1 year ago • 9 comments
trafficstars

Hello, first time contributing to a nix project! I hope this is helpful for users of this distribution of neovim.

When doing my configuration using nixvim I couldn't get dashboard-nvim to work, and when digging deeper I realized that the plugin's configuration hasn't been updated in a bit and it underwent some breaking changes. So this pull request adapts to such breaking changes and gets it back to a working state!

I've tested it both with the default theme hyper:

as well as doom:

Here's with an actual configuration of doom:

I've tried to follow the contribution guidelines and I think I followed it correctly but please do correct me where I may be wrong! Specially code-wise, I am fairly new to the language's specifications!

wizardlink avatar Jan 12 '24 08:01 wizardlink

Thank you very much for your work!

This plugin seems to have been implemented quite a while ago, it does not use the best practices we try to use in the newer implementation. I think it's okay to leave the old options as is (if you don't want to change them), but for all the newer options it would be nice to try and stick with the way we try to do thing:

We want to avoid whenever possible to set a nix default value (default = ....) because this will be added in the user's init.lua, and could slow down neovim. There are a number of helpers in helpers.defaultNullOpts.xxx to create a number of options. It would be best to use them (you can check other plugins for inspiration if needed)

traxys avatar Jan 14 '24 21:01 traxys

Awesome! I will follow it up with some commits for those changes, thanks for pointing it out.

wizardlink avatar Jan 15 '24 08:01 wizardlink

I've done the adaptions @traxys, not too sure about the cases where I used mkNullable, as ideally you'd want to avoid it, but these properties don't have defaults set by the plugin; please let me know if there is a need for further adaptations!

wizardlink avatar Jan 15 '24 14:01 wizardlink

On another note, there isn't any helper function using mkOption where example is set, yet I've seen a few examples where it's used - however in my cases these are nullable whereas those examples weren't, so how important are examples? Or should I just use mkCompositeOption for these submodules and ditch the example property?

wizardlink avatar Jan 15 '24 14:01 wizardlink

I've done the adaptions @traxys, not too sure about the cases where I used mkNullable, as ideally you'd want to avoid it, but these properties don't have defaults set by the plugin; please let me know if there is a need for further adaptations!

mkNullable is perfectly good if upstream has no default value!

traxys avatar Jan 15 '24 14:01 traxys

On another note, there isn't any helper function using mkOption where example is set, yet I've seen a few examples where it's used - however in my cases these are nullable whereas those examples weren't, so how important are examples? Or should I just use mkCompositeOption for these submodules and ditch the example property?

If you want to use a example you would need to skip the helpers wrapper and use the correct mkOption yourself (with the correct type)

traxys avatar Jan 15 '24 14:01 traxys

On another note, there isn't any helper function using mkOption where example is set, yet I've seen a few examples where it's used - however in my cases these are nullable whereas those examples weren't, so how important are examples? Or should I just use mkCompositeOption for these submodules and ditch the example property?

If you want to use a example you would need to skip the helpers wrapper and use the correct mkOption yourself (with the correct type)

But how important is it for the document generation? If it doesn't affects it I'll just use mkCompositeOption.

wizardlink avatar Jan 15 '24 14:01 wizardlink

Providing examples can be nice for users, you can do as you wish, I think we don't often provide examples

traxys avatar Jan 15 '24 14:01 traxys

Made my final changes in regards to submodules, I'm good to go @traxys! Thanks for all the advice!

wizardlink avatar Jan 15 '24 15:01 wizardlink

Could you please rebase this on main ?

GaetanLepage avatar Jun 07 '24 14:06 GaetanLepage

Could you please rebase this on main ?

I missed this when working on #1590. This PR is superseded by that one.

MattSturgeon avatar Jun 07 '24 14:06 MattSturgeon