plugins/flash: switch to mkNeovimPlugin #1714
Conversation
| merged = flatten [ | ||
| opts | ||
| (mapAttrsToList (mode: extras: map (opt: "${mode}.${opt}") (opts ++ extras)) modes) | ||
| ]; |
There was a problem hiding this comment.
Why do you have those variables "out of order" ? I would personally prefer an "imperative-friendly" way.
There was a problem hiding this comment.
Could you clarify what you mean? Do you mean having merged declared before opts?
I did that to make it obvious up front what the goal of the let block is, rather than having rather large attr sets/lists being the first thing you see.
Happy to change and/or remedy with comments.
There was a problem hiding this comment.
Yes, I meant
a = ...
b = f(a)
c = f(b)
d = f(a, c)
...
I find it more readable. Otherwise you are wondering what these opts are. But this is personal preference of course.
There was a problem hiding this comment.
I find it more readable. Otherwise you are wondering what these
optsare. But this is personal preference of course.
Generally, I agree. It was only the size of the other variable definitions that changed my mind this time.
I think because the opts is right there on the next line it is ok (this time)?
There was a problem hiding this comment.
Yes sure, either way is fine. We should stop nit picking (I am looking at myself).
There was a problem hiding this comment.
Yes sure, either way is fine. We should stop nit picking (I am looking at myself).
Nothing wrong with being thorough and discussing the best approach in nuanced cases.
Maybe avoid nit-picking new contributes if it is likely to put them off contributing, but that doesn't mean let sloppy code in 😃
plugins/utils/flash.nix
Outdated
| # TODO: check automatic search config still works | ||
| (mkRemovedOptionModule [ | ||
| "plugins" | ||
| "flash" | ||
| "search" | ||
| "automatic" | ||
| ] "You can configure `plugins.flash.settings.search.*` directly.") |
There was a problem hiding this comment.
@traxys am I missing anything with this option?
You had it set-up to assign helpers.emptyTable when search.automatic = true, but isn't that just explicitly enabling the default behaviour?
There was a problem hiding this comment.
I'm not very familiar with the plugin, I don't remembrer
There was a problem hiding this comment.
I'm also not too familiar, so just trying to interpret the docs.
I think we can go with this and just hope if there's any issues they get reported.
MattSturgeon
force-pushed
the
flash
branch
3 times, most recently
from
5817bdf
to
6e3df10
Compare
|
Is this blocked by anything ? |
No, don't think so |
| # Not sure what the type is... | ||
| # Think it's listOf (maybeRaw (listOf (maybeRaw str)))? |
There was a problem hiding this comment.
This needs to be solved.
There was a problem hiding this comment.
The comment is not mine, it is already present in main.
Same as #1714 (comment), I think I'd have to play with actually using/configuring the plugin to understand to correct solution.
The current type will at least allow users who know how to configure the plugin to do so, though they won't get build-time type checking.
There was a problem hiding this comment.
Ok, I didn't pay attention.
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at aaab869 |
Prompted by changes in #1674, I've gone ahead and updated https://github.com/folke/flash.nvim to use mkNeovimPlugin and rfc42 settings.
The "modes" use the same options as top-level settings, so I've maintained a similar system to the existing implementation where
mkModeOptioncombines all the common settings with any extras.Rather than documenting mode-specific defaults in the option itself, I pass the defaults through to the sub-options themselves. This makes the docs slightly more accurate.
The
search.automaticoption is removed. If my interpretation of upstream's docs is correct it was never needed?