breaking: Rework CreateCommandOpts, CommandInfos and CommandComplete#296
Open
c-c-k wants to merge 1 commit into
Open
breaking: Rework CreateCommandOpts, CommandInfos and CommandComplete#296c-c-k wants to merge 1 commit into
CreateCommandOpts, CommandInfos and CommandComplete#296c-c-k wants to merge 1 commit into
Conversation
bb19716 to
7b96ca2
Compare
Contributor
Author
|
Will drop the v0.10 flags, sec |
7b96ca2 to
059baf6
Compare
059baf6 to
6c884f0
Compare
6c884f0 to
ca69ed4
Compare
5b28554 to
a57f318
Compare
a57f318 to
7ad27a7
Compare
CreateCommandOpts, CommandInfos and CommandComplete on NVIM v0.12.0
CreateCommandOpts, CommandInfos and CommandComplete on NVIM v0.12.0CreateCommandOpts, CommandInfos and CommandComplete
5913040 to
0375a30
Compare
* Add custom serde deserializer for `CommandComplete`. * Rename `CommandComplete::CustomList(fun)` to `CommandComplete::Callback(fun)`. * Add `CommandComplete::Custom(String)`. * Add `CommandComplete::Customlist(String)`. * Add other `CommandComplete` variants listed in the NVIM documentation. * Add `CommandComplete` tests. * Change `CommandInfos::complete`'s type from `String` to `CommandComplete`. * Mark `nvim_oxi::api::get_commands` as unsafe. * Modify `CreateCommandOpts::complete` to accept a function directly. * Modify existing `create_user_command` tests to `get_commands` being unsafe.
0375a30 to
ed6d5c9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR change list
CommandComplete.CommandComplete::CustomList(fun)toCommandComplete::Callback(fun).CommandComplete::Custom(String).CommandComplete::Customlist(String).CommandCompletevariants listed in the NVIM documentation.CommandCompletetests.CommandInfos::complete's type fromStringtoCommandComplete.nvim_oxi::api::get_commandsas unsafe.CreateCommandOpts::completeto accept a function directly.create_user_commandtests toget_commandsbeing unsafe.Rationale
This PR was originally necessary to fix the failing
create_del_user_commandanduser_command_with_counttests because NVIM v0.12 modified a few of the builtin plugin commands to define thecompletefield directly as a Lua function instead of as a"customlist"with acomplete_arg="v:lua..."field.In the process of working on this PR however I realized that the C API
nvim_get_commandsfunction creates a Lua registry slot for each command and completion callback, which it expects the caller to clean up.Currently
nvim-oxidoes not perform this cleanup.Consequentially calling
nvim_oxi::api::get_commandscreates a serious leak of Lua registry slots which in turn also prevents the callbacks from being garbage collected if their commands are deleted.Fixing this will probably require reworking
nvim_oxi::Functioninto freeing the Lua registry slot it contains a reference to when dropped.This in turn will require adjusting the behavior of all the
nvim_oxifunctions that useFunction, which is beyond the scope of this PR.However, since the above solution will require converting all the rawlua_refsreturned bynvim_get_commandsintoFunction's it would be necessary to process the entireDictionaryreturned by the C API function before returning it to the user.In which case I think it also makes sense to change the return type ofnvim_get_commandsfromimpl SuperIterator<CommandInfos>toHashMap<String, CommandInfos>which would also fall in line with the return type of the corresponding functions in the C and Lua API's.edit: After some AI consultation I realize a better solution would probably be to create a custom iterator that would do minimal cleanup processing for any remaining items in the iterator when it's dropped.
Regardless of the above, I think it would be appropriate to rename
CommandComplete::CustomListtoCommandComplete::Callbackso as to useCommandComplete::Customlistfor it's official purpose of holding the name of a Vim Script function.Also I think it would be convenient to allow
CreateCommandOpts::completeto accept a function directly (but still to also acceptCommandComplete).