👍 add plugin unload feature #385
Conversation
Plugin API:
- `Entrypoint` can now return `AsyncDisposable`.
- It is called by `denops#plugin#unload()`.
- It is called when the server closing.
- When `denops#server#close()` is called.
- When `denops#server#stop()` is called.
- When the channel is closed.
- When the server process trapped `SIGINT`.
Vim API:
- Add new `denops-function`:
- `denops#plugin#unload({plugin})`
- Add new events:
- `DenopsPluginUnloadPre:{plugin}`
- `DenopsPluginUnloadPost:{plugin}`
- `DenopsPluginUnloadFail:{plugin}`
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce new functionality to unload Denops plugins, with appropriate events and error handling. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Vim
participant Denops
participant Plugin
User->>Vim: Call denops#plugin#unload(name)
Vim->>Denops: Invoke unload(name)
Denops->>Plugin: Fire DenopsPluginUnloadPre
Plugin-->>Denops: Acknowledge Pre-Unload
Denops->>Plugin: Execute unload logic
Plugin-->>Denops: Unload Success/Fail
Denops->>Plugin: Fire DenopsPluginUnloadPost or DenopsPluginUnloadFail
Plugin-->>Denops: Acknowledge Post-Unload or Handle Fail
Denops->>Vim: Return Status
Vim->>User: Unload Complete/Error
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This breaks `--watch` option.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v7-pre #385 +/- ##
==========================================
+ Coverage 92.33% 92.79% +0.46%
==========================================
Files 21 23 +2
Lines 1291 1361 +70
Branches 144 159 +15
==========================================
+ Hits 1192 1263 +71
+ Misses 96 95 -1
Partials 3 3 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
tests/denops/testdata/dummy_valid_dispose_plugin.ts (1)
1-3: Reminder: Update import statement.The TODO comment indicates that the
Entrypointshould be imported fromdenops-coreonce issue #349 is resolved.// import type { Entrypoint } from "jsr:@denops/[email protected]"; + // TODO: Update import statement once issue #349 is resolved.tests/denops/testdata/dummy_invalid_dispose_plugin.ts (1)
1-3: Reminder: Update import statement.The TODO comment indicates that the
Entrypointshould be imported fromdenops-coreonce issue #349 is resolved.// import type { Entrypoint } from "jsr:@denops/[email protected]"; + // TODO: Update import statement once issue #349 is resolved.denops/@denops-private/plugin.ts (1)
1-1: Address the TODO comment.The TODO comment references a specific issue (#349). It would be helpful to provide more context or a link to the issue for better traceability.
-// TODO: #349 Update `Entrypoint` in denops-core, remove this module from `$.test.exclude` in `deno.jsonc`, and remove this module. +// TODO: Update `Entrypoint` in denops-core (see issue #349), remove this module from `$.test.exclude` in `deno.jsonc`, and remove this module.denops/@denops-private/service.ts (1)
1-2: Reminder: ImportEntrypointfrom denops-core.The TODO comment indicates that the
Entrypointshould be imported fromdenops-corein the future.- // TODO: #349 Import `Entrypoint` from denops-core. + // TODO: #349 Import `Entrypoint` from denops-core.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- autoload/denops/_internal/plugin.vim (3 hunks)
- autoload/denops/plugin.vim (1 hunks)
- deno.jsonc (1 hunks)
- denops/@denops-private/host.ts (3 hunks)
- denops/@denops-private/host/nvim_test.ts (1 hunks)
- denops/@denops-private/host/vim_test.ts (1 hunks)
- denops/@denops-private/host_test.ts (1 hunks)
- denops/@denops-private/plugin.ts (1 hunks)
- denops/@denops-private/service.ts (8 hunks)
- denops/@denops-private/service_test.ts (10 hunks)
- doc/denops.txt (2 hunks)
- plugin/denops.vim (1 hunks)
- plugin/denops/debug.vim (1 hunks)
- tests/denops/runtime/functions/plugin_test.ts (6 hunks)
- tests/denops/testdata/dummy_invalid_dispose_plugin.ts (1 hunks)
- tests/denops/testdata/dummy_valid_dispose_plugin.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- plugin/denops.vim
Additional context used
Biome
denops/@denops-private/plugin.ts
[error] 35-35: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
GitHub Check: codecov/patch
denops/@denops-private/host.ts
[warning] 77-77: denops/@denops-private/host.ts#L77
Added line #L77 was not covered by testsdenops/@denops-private/service.ts
[warning] 231-233: denops/@denops-private/service.ts#L231-L233
Added lines #L231 - L233 were not covered by tests
Additional comments not posted (44)
tests/denops/testdata/dummy_valid_dispose_plugin.ts (1)
5-10: LGTM!The function
mainis correctly defined as anEntrypointand includes an async dispose method that executes a Denops command.tests/denops/testdata/dummy_invalid_dispose_plugin.ts (1)
5-10: LGTM!The function
mainis correctly defined as anEntrypointand includes an async dispose method that throws an error for testing purposes.plugin/denops/debug.vim (1)
12-14: LGTM!The new autocmds for
DenopsPluginUnloadPre,DenopsPluginUnloadPost, andDenopsPluginUnloadFailevents are correctly defined and follow the existing pattern.deno.jsonc (2)
6-6: LGTM!The new task for coverage with exclusions is correctly defined.
10-14: LGTM!The new test exclusions are correctly defined and include a TODO comment for future updates.
denops/@denops-private/host.ts (2)
76-77: Ensure theinvokefunction handles the "unload" case correctly.The
invokefunction has been updated to handle the "unload" case. Ensure that the arguments are correctly passed and handled.Tools
GitHub Check: codecov/patch
[warning] 77-77: denops/@denops-private/host.ts#L77
Added line #L77 was not covered by tests
52-52: Ensure theunloadmethod is implemented correctly.The
unloadmethod in theServiceinterface should be reviewed to ensure it is correctly implemented in the corresponding class.Verification successful
The
unloadmethod is implemented correctly.The
unloadmethod with the signatureasync unload(name: string): Promise<void>is present in theServiceclass located indenops/@denops-private/service.ts. This confirms that the method is correctly implemented.
denops/@denops-private/service.ts:async unload(name: string): Promise<void>Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `unload` method in the `Service` class. # Test: Search for the `unload` method implementation. Expect: Only occurances of the new method. rg --type ts $'unload(name: string): Promise<void>'Length of output: 52
Script:
#!/bin/bash # Description: Locate the `Service` class and verify the implementation of the `unload` method within it. # Step 1: Locate the `Service` class definition. rg --type ts 'class Service' # Step 2: Once the file containing the `Service` class is found, search for the `unload` method implementation within that file. # Note: This command will be executed after identifying the file from the first command. FILE_PATH=$(rg --type ts --files-with-matches 'class Service') if [ -n "$FILE_PATH" ]; then rg --type ts 'unload(name: string): Promise<void>' $FILE_PATH else echo "Service class not found." fiLength of output: 360
Script:
#!/bin/bash # Description: Verify if the `unload` method is implemented with a different signature in the `Service` class. # Search for any method named `unload` within the `denops/@denops-private/service.ts` file. rg --type ts 'unload' denops/@denops-private/service.tsLength of output: 437
denops/@denops-private/host_test.ts (1)
14-14: Ensure the test for theunloadmethod is comprehensive.The test for the
unloadmethod should cover all relevant scenarios, including valid and invalid arguments.Verification successful
The
unloadmethod has comprehensive test coverage.The search results indicate that the
unloadmethod is tested in various scenarios, including unloading a plugin, handling errors, and ensuring a plugin is not unloaded multiple times.
tests/denops/runtime/functions/plugin_test.tsdenops/@denops-private/service_test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for the `unload` method. # Test: Search for the `unload` method test cases. Expect: Only occurances of the new test cases. rg --type ts $'t.step("calls \'unload\'",'Length of output: 129
Script:
#!/bin/bash # Description: Verify the test coverage for the `unload` method. # Test: Search for the `unload` method test cases. Expect: Only occurrences of the new test cases. rg --type ts 't.step("calls \'unload\'",'Length of output: 93
Script:
#!/bin/bash # Description: Verify the test coverage for the `unload` method. # Test: Search for the `unload` method test cases. Expect: Only occurrences of the new test cases. rg --type ts 't.step("calls '\''unload'\''",'Length of output: 129
Script:
#!/bin/bash # Description: Verify the test coverage for the `unload` method. # Test: Search for the `unload` method occurrences in test files. rg --type ts 'unload'Length of output: 4310
denops/@denops-private/host/vim_test.ts (1)
21-21: Integration ofunloadmethod looks good.The
unloadmethod has been correctly added to theserviceobject.autoload/denops/_internal/plugin.vim (6)
4-4: Addition ofSTATE_UNLOADINGis appropriate.The new state
STATE_UNLOADINGis consistent with the existing states.
31-37: Implementation ofdenops#_internal#plugin#unloadlooks good.The function correctly changes the plugin state to
unloadingand notifies the server to invoke theunloadaction.
72-77: Implementation ofs:DenopsSystemPluginUnloadPrelooks good.The function correctly identifies the plugin name, updates its state to
unloading, and triggers the corresponding autocmd event.
79-85: Implementation ofs:DenopsSystemPluginUnloadPostlooks good.The function correctly identifies the plugin name, resets its state to
reserved, clears callbacks, and triggers the corresponding autocmd event.
87-93: Implementation ofs:DenopsSystemPluginUnloadFaillooks good.The function correctly identifies the plugin name, updates its state to
failed, clears callbacks, and triggers the corresponding autocmd event.
100-102: Autocmd events for plugin unloading are correctly added.The new autocmd events for
DenopsSystemPluginUnloadPre,DenopsSystemPluginUnloadPost, andDenopsSystemPluginUnloadFailare correctly defined and nested.denops/@denops-private/host/nvim_test.ts (1)
26-26: Integration ofunloadmethod looks good.The
unloadmethod has been correctly added to theserviceobject.denops/@denops-private/service.ts (9)
Line range hint
44-65:
Modification ofloadmethod looks good.The
loadmethod has been updated to remove thesuffixparameter and correctly handles plugin loading.
68-79: Implementation of#unloadmethod looks good.The private
#unloadmethod correctly handles the unloading of plugins and returns the plugin instance.
81-83: Implementation ofunloadmethod looks good.The public
unloadmethod correctly invokes the private#unloadmethod.
85-89: Implementation ofreloadmethod looks good.The
reloadmethod correctly unloads and reloads the plugin.
149-159: Modification ofclosemethod looks good.The
closemethod correctly unloads all plugins and clears the internal state.
176-178: Addition ofPluginModuletype looks good.The
PluginModuletype correctly defines the structure of a plugin module.
182-183: Addition of#loadedWaiterand#disposablelooks good.The new properties
#loadedWaiterand#disposablecorrectly handle the plugin loading state and resource disposal.
Line range hint
199-224:
Modification of#loadmethod looks good.The
#loadmethod correctly handles plugin loading, including error handling and event emission.
258-266: Implementation ofcreateScriptSuffixlooks good.The
createScriptSuffixfunction correctly generates a suffix for script URLs to handle reloads.doc/denops.txt (5)
412-419: Accurate and clear documentation of plugin loading events.The documentation correctly outlines the sequence of events triggered during the loading of a plugin.
420-434: Accurate and clear documentation of thedenops#plugin#unloadfunction.The documentation correctly describes the function's purpose, behavior, and the sequence of events triggered during plugin unloading.
437-442: Accurate and clear documentation of plugin reloading events.The documentation correctly outlines the sequence of events triggered during the reloading of a plugin and references related functions.
509-526: Accurate and clear documentation of new plugin unloading events.The documentation correctly describes the purpose and timing of the new
DenopsPluginUnloadPre,DenopsPluginUnloadPost, andDenopsPluginUnloadFailevents.
524-526: Accurate and clear documentation of theDenopsPluginUnloadFailevent.The documentation correctly describes the purpose and timing of the
DenopsPluginUnloadFailevent.tests/denops/runtime/functions/plugin_test.ts (9)
17-18: Constants for valid and invalid dispose test scripts are correctly defined.The constants
scriptValidDisposeandscriptInvalidDisposeare correctly defined and will be used in the tests for plugin unloading.
150-184: Comprehensive tests fordenops#plugin#unloadfunction.The tests cover scenarios where the plugin is successfully unloaded, including verifying that the appropriate events are fired and the dispose method is called.
186-223: Comprehensive tests fordenops#plugin#unloadfunction with dispose method failure.The tests cover scenarios where the plugin's dispose method throws an error, including verifying that the appropriate events are fired and the error message is output.
225-248: Comprehensive tests fordenops#plugin#unloadfunction with non-existent plugin.The tests cover scenarios where the plugin is not yet loaded, including verifying that no events are fired and no messages are output.
250-274: Comprehensive tests fordenops#plugin#unloadfunction with already unloaded plugin.The tests cover scenarios where the plugin is already unloaded, including verifying that no events are fired and no messages are output.
Line range hint
295-336: Comprehensive tests fordenops#plugin#reloadfunction.The tests cover scenarios where the plugin is successfully reloaded, including verifying that the appropriate events are fired and the entrypoint is called.
307-346: Comprehensive tests fordenops#plugin#reloadfunction with dispose method failure.The tests cover scenarios where the plugin's dispose method throws an error during reload, including verifying that the appropriate events are fired and the error message is output.
357-371: Comprehensive tests fordenops#plugin#reloadfunction with non-existent plugin.The tests cover scenarios where the plugin is not yet loaded, including verifying that no events are fired and no messages are output.
373-383: Comprehensive tests fordenops#plugin#reloadfunction with already unloaded plugin.The tests cover scenarios where the plugin is already unloaded, including verifying that no events are fired and no messages are output.
denops/@denops-private/service_test.ts (5)
3-3: LGTM!The added imports
assertArrayIncludesandresolvesNextare appropriate for the new test cases.Also applies to: 17-17
31-32: LGTM!The new constants
scriptValidDisposeandscriptInvalidDisposeare necessary for the new test cases.
309-353: LGTM!The new test case for reloading a previously unloaded plugin is comprehensive and ensures correct event emissions.
356-527: LGTM!The new test cases for the
.unload()method cover a wide range of scenarios, ensuring robust handling of plugin unloading.
Line range hint
544-725:
LGTM!The new test cases for the
.reload()method are thorough and ensure that the service handles various reloading scenarios correctly.
| export type Entrypoint = ( | ||
| denops: Denops, | ||
| ) => void | AsyncDisposable | Promise<void | AsyncDisposable>; |
There was a problem hiding this comment.
Avoid using void in union types.
Using void in union types can be confusing. Replace void with undefined for clarity.
-export type Entrypoint = (
- denops: Denops,
-) => void | AsyncDisposable | Promise<void | AsyncDisposable>;
+export type Entrypoint = (
+ denops: Denops,
+) => undefined | AsyncDisposable | Promise<undefined | AsyncDisposable>;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type Entrypoint = ( | |
| denops: Denops, | |
| ) => void | AsyncDisposable | Promise<void | AsyncDisposable>; | |
| export type Entrypoint = ( | |
| denops: Denops, | |
| ) => undefined | AsyncDisposable | Promise<undefined | AsyncDisposable>; |
Tools
Biome
[error] 35-35: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
|
When vim-denops/deno-denops-core#9 is merged, I would like to have it released as |
It is never resolved if it is called between `load()` and `unload()`.
Refs #349, vim-denops/deno-denops-core#8
Plugin API:
Entrypointcan now returnAsyncDisposable.denops#plugin#unload().denops#server#close()is called.denops#server#stop()is called.SIGINT.Vim API:
denops-function:denops#plugin#unload({plugin})DenopsPluginUnloadPre:{plugin}DenopsPluginUnloadPost:{plugin}DenopsPluginUnloadFail:{plugin}Additional
Added tests for script rewrite reload, note that this breaks
deno test --watch.Summary by CodeRabbit