🐛 Refactor denops#plugin module and fix denops#cache#update() to use registered plugins instead
#367
Conversation
WalkthroughSignificant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Vim as Vim
participant Denops as Denops Plugin System
participant Plugin as Plugin
Note over Vim, Denops: Plugin Loading Process
Vim->>Denops: load(plugin_name, script)
Denops->>Plugin: Set plugin state to LOADING
Plugin->>Denops: Script execution result
alt Successfully loaded
Denops->>Plugin: Set state to LOADED
Denops->>Vim: Notify success
else Failed to load
Denops->>Plugin: Set state to FAILED
Denops->>Vim: Notify failure
end
Assessment against linked issues
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
=======================================
Coverage 90.23% 90.23%
=======================================
Files 9 9
Lines 676 676
Branches 64 64
=======================================
Hits 610 610
Misses 66 66 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- autoload/denops/_internal/plugin.vim (1 hunks)
- autoload/denops/cache.vim (1 hunks)
- autoload/denops/plugin.vim (3 hunks)
Additional comments not posted (19)
autoload/denops/cache.vim (5)
1-2: The introduction of constantss:rootands:modis a good practice for values that do not change, enhancing maintainability and preventing accidental reassignments.
6-8: The use ofconstfor local variables withindenops#cache#updateensures that these do not get re-assigned inadvertently, which is a good practice for clarity and error prevention.
Line range hint
6-30: The refactoring of the cache update mechanism to utilize dynamic plugin and entry file lists is a significant improvement. It allows for more flexible cache management and can adapt to changes in the plugin environment more responsively.
Line range hint
6-30: Error handling ins:on_stderrand job cleanup ins:on_exitare well implemented. These are crucial for robustness, especially in asynchronous operations like handling external jobs.
6-8: Ensure that thel:pluginsandl:entryfilesvariables are correctly populated as expected. This new logic integrates plugin management directly with cache updates, which should improve the coherence of state management across the system.autoload/denops/_internal/plugin.vim (6)
1-4: Defining plugin states as constants is a best practice that enhances readability and maintainability of the code by avoiding magic strings.
8-13: The update todenops#_internal#plugin#getto include plugin state and callbacks is crucial for the new event-driven plugin system. This change enables better state management and event handling.
15-17: The modification todenops#_internal#plugin#listto return a mapped list of plugin values ensures that the function does not leak references to its internal state, which is a good security practice.
19-36: The refactor ofdenops#_internal#plugin#loadand the introduction ofdenops#_internal#plugin#reloadare well-structured to handle the new plugin states and improve the reliability of plugin loading and reloading processes.
38-61: The introduction ofs:DenopsSystemPluginPre,s:DenopsSystemPluginPost, ands:DenopsSystemPluginFailfunctions is a solid design choice. These functions facilitate a more modular and event-driven approach to plugin management.
63-69: The autocmd group setup for handling plugin events (DenopsSystemPluginPre,DenopsSystemPluginPost,DenopsSystemPluginFail) is correctly implemented. This setup ensures that plugin events are handled in an organized manner.autoload/denops/plugin.vim (8)
2-2: The update todenops#plugin#is_loadedto check the plugin state using a regex pattern is a straightforward and effective way to determine if a plugin is loaded or failed.
21-38: The refactor ofdenops#plugin#waitto handle plugin loading states more robustly, including better timeout and error handling, is a significant improvement. This change enhances the reliability of the plugin waiting mechanism.
42-49: The changes indenops#plugin#wait_asyncto handle callbacks based on plugin state are well thought out. This ensures that callbacks are only called when appropriate, which is crucial for event-driven programming.
53-57: The delegation of functionality fromdenops#plugin#loadanddenops#plugin#reloadto their counterparts in the internal plugin module simplifies the external API and ensures consistency in plugin handling.
61-71: The refactor ofdenops#plugin#discoverto use a pattern-based approach for discovering plugins is efficient and reduces complexity. This method ensures that all relevant plugins are considered, and the counter provides a simple way to track the number of plugins loaded.
76-77: The changes todenops#plugin#check_typeto utilize the updated plugin listing function reflect good integration with the new plugin management system. This ensures that type checking is performed against the current state of plugins.
93-106: The deprecation ofdenops#plugin#registerin favor ofdenops#plugin#loadis clearly communicated. Including backward compatibility notes and providing a clear migration path are best practices for API changes.
109-118: The introduction ofs:find_scriptto abstract the script finding logic is a clean way to encapsulate this functionality. The error handling with a throw statement ensures that issues are caught early.
It is too slow. Why? |
|
Fixed. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- autoload/denops/_internal/plugin.vim (1 hunks)
- autoload/denops/cache.vim (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- autoload/denops/cache.vim
Additional comments not posted (7)
autoload/denops/_internal/plugin.vim (7)
1-4: Define plugin states as constants for clearer and more maintainable code.This change improves code readability and maintainability by using descriptive constants instead of magic strings throughout the code.
63-69: Review the new autocmd groupdenops_autoload_plugin_internalfor correct event registration and cleanup.The autocmd group setup correctly handles event registration and ensures that the plugin state is reset when the Denops system is closed. This is a good practice for avoiding memory leaks or stale data.
19-27: Validate the updated plugin loading mechanism indenops#_internal#plugin#load.The function now explicitly updates plugin state and script details, which is crucial for correct plugin lifecycle management.
Verification successful
The updated plugin loading mechanism in
denops#_internal#plugin#loadcorrectly updates the plugin state and interacts with the server as intended.
- The function
denops#_internal#plugin#loadsets the plugin state tos:STATE_LOADINGand updates the script details.- It then notifies the server with the
invokecommand and the appropriate arguments.Locations in the codebase:
autoload/denops/_internal/plugin.vim:
- Line setting plugin state to
s:STATE_LOADING: Line 5- Server notification: Line 9
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the new loading mechanism correctly updates plugin states and interacts with the server. # Test: Simulate plugin loading to observe state changes and server notifications. ast-grep --lang vim --pattern $'function! denops#_internal#plugin#load($_, $_) { $$$ }'Length of output: 197
Script:
#!/bin/bash # Find the function definition for denops#_internal#plugin#load rg 'function! denops#_internal#plugin#load' -A 10 # List all occurrences of state updates and server notifications within the plugin.vim file rg 'let l:plugin.state' autoload/denops/_internal/plugin.vim rg 'call denops#_internal#server#chan#notify' autoload/denops/_internal/plugin.vimLength of output: 1369
38-41: Ensure that new event handling functions (s:DenopsSystemPluginPre,s:DenopsSystemPluginPost,s:DenopsSystemPluginFail) work as expected.These functions are designed to handle specific plugin lifecycle events. It's crucial to confirm that they trigger the appropriate system responses without causing unintended side effects.
Also applies to: 43-52, 55-60
Verification successful
The functions
s:DenopsSystemPluginPre,s:DenopsSystemPluginPost, ands:DenopsSystemPluginFailare defined and called within theautoload/denops/_internal/plugin.vimfile. The calls are triggered by correspondingUserautocommands, ensuring that these functions are executed during specific plugin lifecycle events.
s:DenopsSystemPluginPre: Defined at line 38 and called byautocmd User DenopsSystemPluginPre:*.s:DenopsSystemPluginPost: Defined at line 43 and called byautocmd User DenopsSystemPluginPost:*.s:DenopsSystemPluginFail: Defined at line 55 and called byautocmd User DenopsSystemPluginFail:*.These functions are correctly set up to handle their respective events.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct execution and side effects of new event handling functions. # Test: Monitor the system's behavior when these events are triggered. ast-grep --lang vim --pattern $'function! s:DenopsSystemPluginPre() { $$$ }' ast-grep --lang vim --pattern $'function! s:DenopsSystemPluginPost() { $$$ }' ast-grep --lang vim --pattern $'function! s:DenopsSystemPluginFail() { $$$ }'Length of output: 562
Script:
#!/bin/bash # Description: Verify the correct execution and side effects of new event handling functions. # Search for the definition of the functions rg 'function! s:DenopsSystemPluginPre' rg 'function! s:DenopsSystemPluginPost' rg 'function! s:DenopsSystemPluginFail' # Search for the usage of these functions rg 'call s:DenopsSystemPluginPre' rg 'call s:DenopsSystemPluginPost' rg 'call s:DenopsSystemPluginFail'Length of output: 825
8-13: Ensure compatibility and functionality of the modifieddenops#_internal#plugin#getfunction.This function now includes plugin state and callbacks, which could affect how plugins are managed and interacted with. Ensure that this change integrates well with the rest of the system.
15-17: Review the implementation ofdenops#_internal#plugin#listfor potential performance impacts.This function now returns a mapped list of plugin values, potentially increasing computational overhead depending on the number of plugins and their complexity.
30-36: Confirm the correct functionality of thedenops#_internal#plugin#reloadfunction with the new reloading mechanism.This function now handles plugin reloading, which is a critical operation for maintaining plugin functionality during runtime updates.
Verification successful
The
denops#_internal#plugin#reloadfunction correctly handles plugin reloading by retrieving the plugin, logging a debug message, and sending a notification. The implementations of the dependent functionsdenops#_internal#plugin#get,denops#_internal#echo#debug, anddenops#_internal#server#chan#notifyhave been verified and found to be correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test the reliability and correctness of the plugin reloading process. # Test: Trigger plugin reloads under various conditions to ensure robustness. ast-grep --lang vim --pattern $'function! denops#_internal#plugin#reload($_) { $$$ }'Length of output: 195
Script:
#!/bin/bash # Description: Search for the definition of the `denops#_internal#plugin#reload` function. # Search for the function definition in the codebase. rg 'function! denops#_internal#plugin#reload'Length of output: 137
Script:
#!/bin/bash # Description: Extract the full implementation of the `denops#_internal#plugin#reload` function. # Extract the function implementation from the file. sed -n '/function! denops#_internal#plugin#reload/,/endfunction/p' autoload/denops/_internal/plugin.vimLength of output: 433
Script:
#!/bin/bash # Description: Search for the definition of the `denops#_internal#plugin#get` function. # Search for the function definition in the codebase. rg 'function! denops#_internal#plugin#get'Length of output: 131
Script:
#!/bin/bash # Description: Extract the full implementation of the `denops#_internal#plugin#get` function. # Extract the function implementation from the file. sed -n '/function! denops#_internal#plugin#get/,/endfunction/p' autoload/denops/_internal/plugin.vimLength of output: 309
Script:
#!/bin/bash # Description: Search for the definition of the `denops#_internal#echo#debug` function. # Search for the function definition in the codebase. rg 'function! denops#_internal#echo#debug'Length of output: 128
Script:
#!/bin/bash # Description: Extract the full implementation of the `denops#_internal#echo#debug` function. # Extract the function implementation from the file. sed -n '/function! denops#_internal#echo#debug/,/endfunction/p' autoload/denops/_internal/echo.vimLength of output: 236
Script:
#!/bin/bash # Description: Search for the definition of the `denops#_internal#server#chan#notify` function. # Search for the function definition in the codebase. rg 'function! denops#_internal#server#chan#notify'Length of output: 162
Script:
#!/bin/bash # Description: Extract the full implementation of the `denops#_internal#server#chan#notify` function. # Extract the function implementation from the file. sed -n '/function! denops#_internal#server#chan#notify/,/endfunction/p' autoload/denops/_internal/server/chan.vimLength of output: 359
|
It works now. |
1. Move most of core functions of `denops#plugin` into the internal module 2. Track plugin state in `denops#_internal#plugin` before it is loaded
lambdalisue
force-pushed
the
fix-cache-update
branch
from
8a5d20a
to
c616af0
Compare
|
What do you mean "too"? I think it's common to take time for reloading all cache. |
|
Thanks for review & fix. I rebased to tidy commit history so rerequest a review in case @Shougo |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
autoload/denops/_internal/plugin.vim (1)
1-4: Add comprehensive documentation for the new constants.It's essential to document the purpose and usage of these constants to help future developers understand the different states a plugin can be in.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- autoload/denops/_internal/plugin.vim (1 hunks)
- autoload/denops/cache.vim (1 hunks)
- autoload/denops/plugin.vim (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- autoload/denops/cache.vim
- autoload/denops/plugin.vim
Additional comments not posted (2)
autoload/denops/_internal/plugin.vim (2)
15-17: Looks good as it effectively retrieves all plugins.
55-61: Properly handles plugin failures by setting the state and clearing callbacks.
It is normal behavior? |
|
I have fixed for rabbit review. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- autoload/denops/_internal/plugin.vim (1 hunks)
Additional comments not posted (5)
autoload/denops/_internal/plugin.vim (5)
1-4: Introduced constants for plugin states enhance readability and maintainability.Nice use of constants to define plugin states, which makes the code easier to understand and maintain.
22-30: Optimize external call error handling indenops#_internal#plugin#load.Consider adding error handling around external calls to
denops#_internal#echo#debuganddenops#_internal#server#chan#notifyas previously suggested to enhance robustness.
33-39: Replicate error handling improvements indenops#_internal#plugin#reload.Similar to the load function, it's crucial to handle potential errors from
denops#_internal#echo#debuganddenops#_internal#server#chan#notifyto prevent issues during plugin reloading.
41-44: Refactor event handling functions to streamline plugin lifecycle management.The addition of
s:DenopsSystemPluginPre,s:DenopsSystemPluginPost, ands:DenopsSystemPluginFailfunctions along with corresponding autocmds effectively manage plugin states and callbacks, enhancing the plugin lifecycle management.Also applies to: 46-55, 58-63
66-72: Autocmd group for handling plugin events ensures clean state management.The use of an autocmd group to handle plugin-specific events and to reset the plugins state upon
DenopsClosedis a good practice, ensuring that the plugin states are managed cleanly.
I'm sorry but the rabbit review didn't make sense so we should ignore. I removed that commits and force pushed. |
lambdalisue
force-pushed
the
fix-cache-update
branch
from
2f30e6b
to
c616af0
Compare
Close #360. Additionally
denops#plugin#check_typealso discover/find plugins as likedenops#cache#updatethus this PR fixed the behavior to use registered plugins instead.Summary by CodeRabbit
New Features
Refactor
Bug Fixes