refactor!: :ZPack with subcommands#21
Conversation
There was a problem hiding this comment.
Thank you for the contribution, and the link to best practices. I agree this is a better approach going forward!
For a smoother migration experience, I think we should keep cmd_prefix working during the deprecation period — keep registering the prefixed commands (:ZUpdate, :ZClean, …) alongside the new :ZPack interface instead of removing them outright.
The other deprecated options (confirm, disable_vim_loader, plugins_dir) all keep functioning while emitting a warning, so I'd suggest moving cmd_prefix into deprecation.deprecated and doing the same here. The new Sub table already isolates each subcommand's logic, so the legacy commands should just be a small registration shim on top of it — I think a bit of temporary code is a fair trade for not breaking existing setups.
One thing to watch on the warning itself: cmd_prefix defaulted to 'Z', so most users never set it explicitly — a setup-time warning keyed on the option being passed would miss them entirely. I'd suggest emitting the deprecation notice when a legacy command is actually invoked (once, deduped), so it reaches everyone still on the old commands.
Could we also extend the new deprecation test to assert the legacy commands are still created and that invoking one emits the warning?
|
Ok, Sounds good. |
e3b9fae to
c3fb251
Compare
Non-blocking issues surfaced while reviewing the :ZPack subcommand refactor and its cmd_prefix deprecation shim. commands.lua: - complete_command strips the command word explicitly, so a bang-attached subcommand (:ZPack!load) no longer mis-parses and offers subcommand names at the plugin-name position - the dispatcher rejects a bang on subcommands that ignore it (update/restore/clean), restoring the feedback the old :ZUpdate! E477 gave - the dispatcher rejects extra positional args instead of joining them into a misleading 'Plugin "a b" not found in spec' error - validate_name type-checks its input, so a non-string cmd_prefix/cmd_name no longer throws and aborts setup() - setup_legacy threads the resolved cmd_name into legacy command descs and warnings instead of hardcoding :ZPack - setup_legacy emits an error notice for an invalid cmd_prefix instead of returning silently - a startup assert keeps SUB_ORDER in sync with the Sub table - the cmd_prefix deprecation warning dedups once total, not once per command tests: - repurpose cmd_prefix_test.lua as an explicit legacy-shim test: rename the describe block, assert the deprecation warning fires, drop the rejection cases duplicated by cmd_name_test.lua, and fix delete_zpack_commands cleanup calls that passed the prefix as the cmd_name positional - add dispatch_test.lua covering the bang/extra-arg/completion fixes - deprecation_test asserts the warning dedups across different legacy commands - drop the :ZPack-creation test duplicated from cmd_name_test.lua
Replace the prefixed commands
:ZUpdate,:ZRestore,:ZClean,:ZBuild,:ZLoad,:ZDeletewith a single:ZPackcommand that dispatches toupdate,restore,clean,build,load, anddeletesubcommands.Bang and plugin-name args carry over (e.g.
:ZPack! load alpha), and subcommand + plugin-name tab completion works at both levels.The
cmd_prefixsetup option is replaced bycmd_name(defaultZPack). The old option is registered indeprecation.removedand shows a migration warning if setup.BREAKING CHANGE:cmd_prefixno longer has any effect; use the newcmd_nameinstead.More info on best practices: https://github.com/lumen-oss/nvim-best-practices#speaking_head-user-commands
You can try it directly from my fork