fidget-nvim setupOpts update and haskell-tools fixes #1306
Conversation
imnotpoz
requested review from
NotAShelf,
Soliprem and
horriblename
as code owners
| default = false; | ||
| visible = false; | ||
| apply = warn '' | ||
| Option `vim.visuals.fidget-nvim.setupOpts.integration.xcodebuild-nvim.enable` | ||
| has been deprecated upstream. Use | ||
| `vim.visuals.fidget-nvim.setupOpts.notification.window.avoid = ["TestExplorer"]` instead. | ||
| ''; |
There was a problem hiding this comment.
This is not the correct way to warn on an option. You'll want to use mkRemovedOptionModule as the rest of the codebase. Apply does work, but it's a really fragile way of doing this. Also applies to the vim.visuals.fidget-nvim.setupOpts.integration.nvim-tree.enable warning above.
There was a problem hiding this comment.
unfortunately not possible due to mkPluginSetupOption, see #1305
| tabstop = mkOption { | ||
| description = "Width of each tab character in the notification window"; | ||
| type = int; | ||
| default = 8; | ||
| }; | ||
| avoid = mkOption { | ||
| description = "Filetypes the notification window should avoid"; | ||
| type = listOf str; | ||
| default = []; |
There was a problem hiding this comment.
Please order mkOption args as
- type
- default
- (optional) example
- Description
Also fwiw the setupOpts block is freeform so new options are not strictly necessary. Though if you do want to type-check those or for those to show up on documentation, it's fine.
There was a problem hiding this comment.
every single option in this module is ordered description -> type -> default, should I change all of them to this ordering?
NotAShelf
left a comment
NotAShelf
left a comment
There was a problem hiding this comment.
A few comments on formatting and introduction of new options. I'd appreciate it if you could follow the "convention" for mkOption.
Also, this requires a changelog entry after v0.9. LGTM otherwise. Though CC @horriblename for the HLS changes.
changelog for both commits? I can add it once the comments above are resolved |
horriblename
left a comment
horriblename
left a comment
There was a problem hiding this comment.
I'm pretty this will trigger the deprecation warning regardless of whether the user has set this option
I think removing the default value works? (I don't remember if it allows not giving a default without one)
| # see https://github.com/mrcjkb/haskell-tools.nvim/blob/v6.2.0/lua/haskell-tools/lsp/init.lua#L131-L173 | ||
| # for the real ones | ||
| hls = { | ||
| enable = false; |
There was a problem hiding this comment.
from my understanding, we're relying on haskell-tools to start the lsp, so we should set this to false? (enable = true, which is the default, calls vim.lsp.enable("server_name")
3 participants
found these in
:checkhealthSanity Checking
nix fmt).#nix(default package).#maximal.#docs-html(manual, must build).#docs-linkcheck(optional, please build if adding links)x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.