Support plugin/flags.vim file sourced on first flag access#244
Conversation
|
FWIW, this could add a bit of overhead trying to source a plugin/flags.vim file, but only in uncommon corner cases because most current plugins that will have any flags configured will already have Also this approach is a little simpler than what I described in #189 since instead of trying to be clever and queue up flags operations it just tries to ensure flags.vim is sourced before any flags operations are performed to pick up default values. This shouldn't preclude later improvements to do the clever queuing if we're able to get all the intricacies right. |
Partially fixes #189 (allowing plugins to start migrating to plugin/flags.vim but not discouraging the instant/ equivalent yet).
malcolmr
left a comment
malcolmr
left a comment
There was a problem hiding this comment.
If I understand this correctly:
- This is a no-op for any plugin with an
instant/flags.vim; everything proceeds as normal. - For any plugin without
instant/flags.vim, this will trigger an extra load attempt if a caller (not Maktaba) tries to look up its flags (e.g. by using Glaive). But it's unlikely that there are many/any Maktaba plugins that both don't have any flags at all and have a caller that tries to look at their flags. - This does assume that flags are only defined in
instant/flags.vimspecifically (previously they could have been in anyinstant/file); I suspect that's a reasonable assumption though.
| " maktaba#plugin#Get('myplugin').flags.foo.Get() | ||
| " < | ||
| " except that the second version will not trigger flags to load if they haven't | ||
| " been loaded already. |
There was a problem hiding this comment.
If flags haven't been loaded yet, does that mean that flags.foo.Get() will throw some error (which one?) if the foo flag hasn't been set explicitly by the user (but wouldn't if it has?).
What's the right pattern for a library to use if it wants to access flags before flag loading? (Presumably this should also be an uncommon case outside of Maktaba proper(?); should we say that?).
There was a problem hiding this comment.
Right, I want to go a little further to discourage accessing .flags, possibly rename it to ._flags and provide a .Flags() lookup method instead, but I don't want to get too verbose with usage contracts just yet in all the files I'd need to touch for that. The helpers ought to fully encapsulate having the flags resolved before letting you Set/Get them and I can't think of any reason to support poking at them partially initialized.
| " maktaba#plugin#Get('myplugin').Flag('foo') | ||
| " maktaba#plugin#Get('myplugin').flags.foo.Get() | ||
| " < | ||
| " except that the second version will not trigger flags to load if they haven't |
There was a problem hiding this comment.
Note that this is (technically) a breaking change for existing callers. I suspect that's probably fine, though.
There was a problem hiding this comment.
In a sense yes, but to actually break anything you'd need to update the plugins to move from instant/flags.vim to plugin/flags.vim.
| let l:plugin.flags.instant = maktaba#flags#Create('instant', {}) | ||
| endif | ||
|
|
||
| " Load flags file first. |
There was a problem hiding this comment.
Actually I noticed this ApplySettings case probably also needs to EnsureFlagsLoaded first. I'm going to try to get some vroom coverage to repro an issue there along with adding the call to fix it.
There was a problem hiding this comment.
Alright, pushed 0382ab2 to add the missing corner case handling (in Setting.Apply) and test coverage.
Ensures that a plugin/flags.vim file gets loaded before HasFlags or Setting.Apply calls and adds vroom test coverage for more of those cases. Also defines a related AllFlags helper on the plugin object to look up flags if needed and return them.
|
Ready for another look after various updates in 0382ab2 to add test coverage and fix ensuring flags file loaded in two cases I'd neglected ( |
2 participants
Partially fixes #189 (allowing plugins to start migrating to plugin/flags.vim but not discouraging the instant/ equivalent yet).