Refactor options service #602
Conversation
AlexPl292
requested review from
lippfi and
AlexPl292
and removed request for
lippfi
| ) { | ||
| (injector.optionGroup.getOption(IjOptionConstants.ideajoin) as? ToggleOption)?.let { option -> | ||
| injector.optionGroup.setToggleOption(option, OptionScope.GLOBAL) | ||
| } | ||
| } |
There was a problem hiding this comment.
Now it has become more complicated and requires more letters. It would be nice to have something more compact for more comfortable interaction with options.
There was a problem hiding this comment.
I've made some changes to make it a little easier to set options - specifically, introduced getKnownOption, GetKnownStringOption and GetKnownToggleOption. These return an Option, do not return null, and can be passed straight to the helper functions to set the option value, set/unset toggles, etc. They must be called with a known option name or they will fail/throw.
It's a little more work than passing a string name in, but not by much. Using a name based API requires an extra parameter to make the validation error messages work for user facing functionality, and most code that sets options wouldn't have an appropriate value available. And since there are very few places that need to set options, compared to reading options, it's not worth adding an extra set of name based extension functions/overloads to fetch the option and then fetch the value of the option. We can let the caller do this, and it can either do validation of the returned option if it wants, or use the getKnownOption functions to show intent and fetch a well-known option that it knows is always available, such as the options listed in OptionConstants.
Cast succeeds, but only due to erasure
Moves ToggleOption to the proper package, leaving behind a skeleton class to keep ideavim-sneak and IdeaVim-EasyMotion happy. Removes NumberOption and related number OptionsManager properties which are only used by which-key 0.6.2, because this plugin is already broken due to a separate API change. (The plugin is expecting the command trie to use `CommandPartNode<ActionBeanClass>`, but it's actually using `CommandPartNode<VimActionsInitiator>`). Also removes `ToggleOption.value`, used by which-key
No external usages
Leaves a skeleton, deprecated implementation of OptionService for existing external use by plugins
Moves functions that require validation parameters to extension functions. Core interface should not require additional user facing paramter
The interface now has a simple setter and getter, as well as other functions for managing options. More friendly functions for getting and setting values are available as extension functions
citizenmatt
force-pushed
the
refactor/options-obsolete-api
branch
from
6755cb0
to
2de7d72
Compare
|
Force pushed rebased against current |
|
Thanks a lot for your work with this PR! |
This PR is a set of refactorings on the options service:
OptionsManager.com.maddyhome.idea.vim.vimscript.services.OptionValueAccessortocom.maddyhome.idea.vim.options.OptionValueAccessor. Some code still used by extensions is left in place, marked as deprecated, but with enough functionality forwarded to new code, e.g.ToggleOption, replaced with a new instance ofToggleOptionin the correct package.VimOptionGroupand expose on injector. Can no longer useOptionsServiceas this is still used by external code, and now marked as deprecated.VimOptionGroup):setcommand specific validation out of the interface and into the implementation ofSetCommand. Internal callers no longer need to pass in a "token" used for user facing exception messages when getting or setting option values