💪 Minor refactorings #298
💪 Minor refactorings #298
Conversation
These functions are not documented and not necessary.
Because vint does not support Vim 9 syntax like `const`.
WalkthroughThe update refactors function calls and variable declarations across various Vim script and TypeScript files. It standardizes the use of constants, updates function signatures, and enhances type checking. Notably, internal server communication methods are centralized, and dependency versions are bumped up. The changes streamline the codebase, improving maintainability and consistency. Changes
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? TipsChat with CodeRabbit Bot (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
==========================================
+ Coverage 92.59% 96.47% +3.87%
==========================================
Files 3 3
Lines 81 85 +4
Branches 6 6
==========================================
+ Hits 75 82 +7
+ Misses 6 3 -3 ☔ 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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (17)
- autoload/denops.vim (2 hunks)
- autoload/denops/_internal/path.vim (2 hunks)
- autoload/denops/_internal/server/chan.vim (2 hunks)
- autoload/denops/_internal/server/proc.vim (1 hunks)
- autoload/denops/plugin.vim (2 hunks)
- autoload/denops/server.vim (2 hunks)
- autoload/health/denops.vim (5 hunks)
- denops/@denops-private/error.ts (1 hunks)
- denops/@denops-private/host/base.ts (1 hunks)
- denops/@denops-private/host/invoker.ts (1 hunks)
- denops/@denops-private/host/nvim.ts (4 hunks)
- denops/@denops-private/host/vim.ts (4 hunks)
- denops/@denops-private/impl.ts (4 hunks)
- denops/@denops-private/impl_test.ts (1 hunks)
- denops/@denops-private/service.ts (2 hunks)
- denops/@denops-private/util.ts (1 hunks)
- denops/@denops-private/worker/script.ts (4 hunks)
Files skipped from review due to trivial changes (2)
- autoload/denops/_internal/server/proc.vim
- denops/@denops-private/host/invoker.ts
Additional comments: 32
denops/@denops-private/util.ts (1)
- 1-9: The
isMetapredicate is correctly defined and should be used consistently across the codebase to validateMetaobjects.denops/@denops-private/error.ts (1)
- 1-1: The import statement for
ishas been updated to versionv3.11.0. Ensure that this version bump does not introduce any breaking changes or incompatibilities with the existing code.autoload/denops/_internal/path.vim (1)
- 1-2: The variable declarations have been correctly updated to use
constand uppercase names, which is a best practice for constants.autoload/denops.vim (1)
- 2-5: The function calls within
denops#notifyhave been updated to use the newdenops#_internal#server#chan#notify. Ensure that this new function is defined and works as expected.Verification successful
The
denops#_internal#server#chan#notifyfunction is indeed defined in the fileautoload/denops/_internal/server/chan.vim. This confirms that the updated function call indenops#notifyis referencing an existing function within the codebase.
- The function
denops#_internal#server#chan#notifyis defined with two parameters, which aligns with the updated call indenops#notify.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new internal server communication functions are defined and work as expected. ast-grep --lang vim --pattern $'function! denops#_internal#server#chan#notify($_, $_, $_)'Length of output: 198
Script:
#!/bin/bash # Search for the definition of denops#_internal#server#chan#notify in Vim script files rg 'function! denops#_internal#server#chan#notify\('Length of output: 164
denops/@denops-private/impl.ts (3)
1-1: The import statement for
ensureandishas been correctly added to the file.16-16: The new constant
isBatchReturnhas been introduced for type validation. Ensure that it is used correctly wherever batch returns are processed.Verification successful
Based on the verification script's output, the new constant
isBatchReturnis used correctly in the codebase to validate batch returns. The usage is found inimpl.tsandhost/vim.ts.
* 46-47: The `async` keyword has been removed from the `call` method, which implies a change to synchronous execution. Verify that this change does not affect the expected behavior of the method.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `isBatchReturn` is used correctly in the codebase. ast-grep --lang typescript --pattern $'ensure($_, isBatchReturn)'Length of output: 245
Verification successful
The removal of the
asynckeyword from thecallmethod inimpl.tsdoes not affect the expected behavior of the method, as it still returns a Promise and can be used withawaitin asynchronous contexts. The method's asynchronous nature is preserved, and it can be awaited upon in other parts of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the removal of `async` from the `call` method does not affect the expected behavior. ast-grep --lang typescript --pattern $'call($_, $$$)'Length of output: 54
Script:
#!/bin/bash # Search for the `call` method within the class where it is defined to understand the changes made to it. ast-grep --lang typescript --pattern $'class $_ { $$$ call($_, $_) { $$$ } $$$ }'Length of output: 96
Script:
#!/bin/bash # Use ripgrep to search for the `call` method within TypeScript files to locate its definition and usage. rg --type ts 'call\('Length of output: 3702
denops/@denops-private/host/vim.ts (2)
- 1-4: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-5]
The import statements for
ensureandishave been correctly added to the file.
- 10-12: New type checks using
is.TupleOfhave been introduced. Ensure that these are used correctly throughout the codebase.Verification successful
The shell script output indicates that the
is.TupleOftype checks are used in several places within the codebase:
isCallReturnandisBatchReturnindenops/@denops-private/host/vim.tsisInvokeMessageindenops/@denops-private/host/vim.tsisNvimCallFunctionReturnindenops/@denops-private/host/nvim.tsisBatchReturnindenops/@denops-private/impl.tsThe presence of these type checks in multiple files suggests that the new type checks have been integrated into the codebase. However, to ensure they are used correctly, we would need to verify that the places where these constants are used are consistent with the type definitions provided.
Since the review comment specifically asks to ensure that the new type checks using
is.TupleOfare used correctly throughout the codebase, we should check the usage of these constants in the codebase.Let's generate a script to search for the usage of
isCallReturn,isBatchReturn, andisInvokeMessageto verify their correct application.
The new context from the shell scripts shows the usage of
isCallReturn,isBatchReturn, andisInvokeMessagewithin the codebase:
isBatchReturnis defined and used indenops/@denops-private/impl.tsto ensure the type ofresult.isCallReturnandisBatchReturnare defined indenops/@denops-private/host/vim.ts, andisCallReturnis used to ensure the type ofresult.isBatchReturnis also used indenops/@denops-private/host/vim.tsto ensure the type ofresult.isInvokeMessageis defined indenops/@denops-private/host/vim.tsand used in a conditional check.The usage of these type checks in the codebase aligns with their definitions, indicating that they are being used correctly to validate the types of variables as intended. The
ensurefunction is applied withisCallReturnandisBatchReturnto validate the structure ofresult, andisInvokeMessageis used in a conditional to check the message type.Based on the provided context, it appears that the new type checks using
is.TupleOfare indeed used correctly throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new type checks using `is.TupleOf` are used correctly. ast-grep --lang typescript --pattern $'is.TupleOf($$$)'Length of output: 1314
Script:
#!/bin/bash # Check the usage of isCallReturn, isBatchReturn, and isInvokeMessage rg 'isCallReturn|isBatchReturn|isInvokeMessage' --type tsLength of output: 753
denops/@denops-private/host/nvim.ts (2)
- 1-5: The import statement for
ensurehas been updated to the new version. Ensure that this version bump does not introduce any breaking changes or incompatibilities with the existing code.Verification successful
The import statement for
ensurehas been updated to the new version, and the search did not find any references to the old version3.10.0in the TypeScript files. This suggests that the version bump is consistent throughout the codebase.* 15-23: The new constant `isNvimCallFunctionReturn` has been added for type validation. Ensure that it is used correctly wherever Nvim call function returns are processed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new version of the `ensure` module does not introduce breaking changes. rg --type typescript "https://deno.land/x/[email protected]"Length of output: 97
Script:
#!/bin/bash # Search for references to the old version of the `ensure` module. rg "https://deno.land/x/[email protected]"Length of output: 44
Verification successful
The new constant
isNvimCallFunctionReturnis used correctly in the codebase for type validation in the filenvim.ts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `isNvimCallFunctionReturn` is used correctly in the codebase. ast-grep --lang typescript --pattern $'ensure($_, isNvimCallFunctionReturn)'Length of output: 183
autoload/denops/server.vim (2)
1-3: The status constants have been correctly changed from variables to constants, which is a best practice for immutable values.
1-3: The functions
denops#server#notifyanddenops#server#requesthave been removed. Ensure that their removal does not break any existing functionality and that all references to these functions have been updated.Verification successful
Based on the executed scripts, there are no remaining references to the removed functions
denops#server#notifyanddenops#server#requestin the codebase. This suggests that their removal is unlikely to break existing functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the removal of `denops#server#notify` and `denops#server#request` does not break any existing functionality. rg --type vim "denops#server#notify" rg --type vim "denops#server#request"Length of output: 74
Script:
#!/bin/bash # Search for references to the removed functions in TypeScript files rg --type ts "denops#server#notify" rg --type ts "denops#server#request" # Additionally, check for dynamic calls that might be constructed at runtime rg --type vim "server#notify" rg --type vim "server#request" rg --type ts "server#notify" rg --type ts "server#request"Length of output: 192
denops/@denops-private/worker/script.ts (3)
1-1: The import statement for
ensureandishas been correctly updated to the new version.19-22: The
isMessageDatapredicate has been introduced for type checking. Ensure that it is used correctly to validate message data throughout the codebase.Verification successful
To further verify the correct usage of
isMessageDatathroughout the codebase, we should look for other instances where message data is handled and ensure thatisMessageDatais used for validation in those cases as well. Let's search for any other occurrences ofevent.datathat might require validation withisMessageData.
Based on the search results, the
isMessageDatapredicate is used correctly to validateevent.datain thescript.tsfile, and there are no other occurrences ofevent.datathat require validation withisMessageDatain TypeScript files within the codebase. The review comment has been verified and the usage ofisMessageDatais correct as per the provided context.* 25-27: The `emit` function has been introduced to handle emitting events. Ensure that this function is used correctly and does not introduce any side effects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `isMessageData` is used correctly in the codebase. ast-grep --lang typescript --pattern $'ensure($_, isMessageData)'Length of output: 183
Script:
#!/bin/bash # Search for occurrences of `event.data` to verify if `isMessageData` is used for validation. rg "event.data" --type tsLength of output: 137
Verification successful
Based on the provided script output, the
emitfunction is used correctly in the codebase to emit events related to plugin lifecycle. There is no direct evidence of side effects from the usage of theemitfunction in the provided context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `emit` function is used correctly in the codebase. ast-grep --lang typescript --pattern $'emit($_, $_)'Length of output: 382
autoload/denops/_internal/server/chan.vim (3)
1-5: The refactoring to use constants for host determination and RPC function names is a good practice for maintainability and readability.
75-76: The addition of a condition to check
g:denops#disabledbefore proceeding with notify operations is a good defensive programming practice. Ensure that the global variableg:denops#disabledis documented and its usage is consistent across the codebase.Verification successful
The usage of the global variable
g:denops#disabledis consistent across the codebase. It is checked before proceeding with operations inserver.vim, and it is set inproc.vimandchan.vim. This confirms that the addition of the condition inchan.vimaligns with the existing usage pattern of this variable.
- The variable is checked before starting the server and before connecting in
server.vim.- It is set within
proc.vimandchan.vim, which likely serves as a mechanism to disable the server or channel operations under certain conditions.The review comment is therefore consistent with the codebase, and the usage of
g:denops#disabledappears to be well-integrated.* 84-85: Similar to the previous comment, the check for `g:denops#disabled` before request operations is approved. Verification for consistent usage is recommended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of `g:denops#disabled` across the codebase for consistency. rg --type vim 'g:denops#disabled'Length of output: 500
Verification successful
The verification script has confirmed that the check for
g:denops#disabledis used consistently across various files in the codebase. The instances found inserver.vim,_internal/server/proc.vim, and_internal/server/chan.vimindicate that the check is a common pattern before proceeding with server-related operations or setting the variable to disable functionality.Based on the output, the review comment is consistent with the codebase, and the usage of
g:denops#disabledis approved and verified.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Reuse the previous verification script to check for `g:denops#disabled`. rg --type vim 'g:denops#disabled'Length of output: 500
autoload/health/denops.vim (7)
1-3: The change to use constants for version declarations is a good practice for maintainability and readability.
49-49: Using the constant
s:DENO_VERSIONin the version comparison function is correct and improves maintainability.58-61: The use of
s:DENO_VERSIONin the error message for an unsupported Deno version is consistent with the constant declaration.71-71: Using the constant
s:VIM_VERSIONin the version comparison function is correct and improves maintainability.77-80: The use of
s:VIM_VERSIONin the error message for an unsupported Vim version is consistent with the constant declaration.90-90: Using the constant
s:NEOVIM_VERSIONin the version comparison function is correct and improves maintainability.96-99: The use of
s:NEOVIM_VERSIONin the error message for an unsupported Neovim version is consistent with the constant declaration.denops/@denops-private/service.ts (3)
2-2: The version bump for
unknownutilfromv3.10.0tov3.11.0is noted. It's important to check the module's changelog for any breaking changes that could affect the codebase.174-177: Similar to the
redrawfunction, the removal ofasyncandawaitfrom thecallfunction should be verified to ensure it does not introduce any unintended side effects.185-189: The removal of
asyncandawaitfrom thedispatchfunction should be verified to ensure it does not introduce any unintended side effects.denops/@denops-private/impl_test.ts (1)
- 13-30: The addition of a new test case for
denops.redraw()with different parameters is a good practice to ensure the function behaves as expected. It's important to ensure that the test covers all relevant cases and that the assertions are correct.autoload/denops/plugin.vim (2)
83-83: The change from
denops#server#requesttodenops#_internal#server#chan#notifyindenops#plugin#reloadfunction alters the behavior from a request-response pattern to a notification pattern. Ensure that this change in control flow is intentional and that the lack of a response from the server will not affect the logic of the plugin reloading process.143-143: The update in
s:registerfunction to usedenops#_internal#server#chan#notifyshould be checked for consistency with the new internal server communication method. Confirm that the notification mechanism is appropriate for the plugin registration process.Verification successful
The search results confirm that the
denops#_internal#server#chan#notifyfunction is used in multiple places within the codebase, including thedenops#plugin#reloadands:registerfunctions inautoload/denops/plugin.vim, as well as inautoload/denops.vim. The presence of a definition fordenops#_internal#server#chan#notifyinautoload/denops/_internal/server/chan.vimsuggests that the function is indeed part of the internal server communication mechanism.Based on the provided output, the update in the
s:registerfunction to usedenops#_internal#server#chan#notifyis consistent with the usage of the new internal server communication method across the codebase. The notification mechanism appears to be appropriately used for the plugin registration process.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new notify function is used correctly across the codebase. rg --type vim 'denops#_internal#server#chan#notify'Length of output: 429
| redraw: (force) => { | ||
| assert(force, is.OneOf([is.Boolean, is.Nullish])); | ||
| return await service.host.redraw(!!force); | ||
| return service.host.redraw(!!force); |
There was a problem hiding this comment.
The output from the scripts provides the following insights:
- In
vim.ts, theredrawmethod is explicitly returningPromise.resolve(), indicating that it is intended to be an asynchronous method, even though the actual operation (this.#client.redraw(force)) might be synchronous. - In
nvim.ts, theredrawmethod also returnsPromise.resolve(), and it includes a comment stating "Do NOTHING on Neovim", which suggests that the method is a no-op but still maintains an asynchronous signature.
Given this information, the redraw method is expected to be asynchronous across different host implementations (vim.ts and nvim.ts). Therefore, the removal of async from the redraw function in service.ts could potentially lead to inconsistencies and unintended side effects, as the function was previously expected to return a promise.
Based on the evidence:
- The
redrawfunction is part of an interface that expects it to be asynchronous (Promise<void>). - The actual implementations in
vim.tsandnvim.tsare asynchronous, returningPromise.resolve().
The review comment is correct in suggesting that the change to synchronous execution in service.ts should be carefully evaluated to ensure it does not introduce any unintended side effects, especially since the function was previously expected to return a promise.
Analysis chain
The removal of async and await from the redraw function suggests a shift to synchronous execution. Ensure that this change does not introduce any unintended side effects, especially if the function was previously expected to return a promise.
To further verify the consistency of the redraw function's execution model across the codebase, we need to examine the implementation of service.host.redraw to determine if it returns a promise.
Let's refine our search to specifically check the redraw method implementations in the vim.ts and nvim.ts files, which are likely to be the classes that implement the host interface.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the `redraw` function is not expected to return a promise in the codebase.
rg --type ts 'redraw\('
Length of output: 848
Script:
#!/bin/bash
# Check the implementation of `service.host.redraw` to see if it returns a promise.
ast-grep --lang typescript --pattern 'class $_ { $$$ redraw($_) { $$$ } $$$ }'
Length of output: 80
Script:
#!/bin/bash
# Check the implementation of `redraw` in `vim.ts` and `nvim.ts` files.
rg --type ts 'redraw\(' -A 3 -g 'vim.ts'
rg --type ts 'redraw\(' -A 3 -g 'nvim.ts'
Length of output: 617
| batch: (...calls) => { | ||
| assert(calls, is.ArrayOf(isCall)); | ||
| return await service.host.batch(...calls); | ||
| return service.host.batch(...calls); |
There was a problem hiding this comment.
The search results confirm that the batch function is declared with the async keyword in both vim.ts and nvim.ts within the denops/@denops-private/host/ directory. This indicates that the batch function is indeed expected to be an asynchronous function that returns a promise.
Given this information, the removal of the async keyword from the batch function in denops/@denops-private/service.ts could potentially introduce side effects, as the function is expected to return a promise due to its asynchronous nature in the host classes.
Therefore, the review comment is correct in suggesting that the removal of async and await from the batch function should be verified to ensure it does not introduce any unintended side effects.
- The
batchfunction is expected to be asynchronous and return a promise, as indicated by its declaration indenops/@denops-private/host/vim.tsanddenops/@denops-private/host/nvim.ts. - The removal of
asyncfrom thebatchfunction indenops/@denops-private/service.tsis inconsistent with its usage in the codebase and could lead to issues if the function is expected to handle asynchronous operations.
Analysis chain
The removal of async and await from the batch function should be verified to ensure it does not introduce any unintended side effects.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the `batch` function is not expected to return a promise in the codebase.
rg --type ts 'batch\('
Length of output: 1010
Script:
#!/bin/bash
# Check the implementation of `service.host.batch` in the host classes.
ast-grep --lang typescript --pattern $'class $_ { $$$ async batch($_, $_) { $$$ } $$$ }'
Length of output: 89
Script:
#!/bin/bash
# Inspect the contents of the files where `async batch` was declared to understand the expected return type.
rg --type ts 'async batch' denops/@denops-private/host/
Length of output: 158
| batch( | ||
| ...calls: (readonly [string, ...unknown[]])[] | ||
| ): Promise<readonly [unknown[], string]>; |
There was a problem hiding this comment.
The verification process has identified an inconsistency:
- The method signature for
batchindenops/@denops/denops.tsdoes not usereadonlytuples, which does not match the updated signature indenops/@denops-private/host/base.ts.
This should be addressed to ensure consistency across the codebase.
Analysis chain
The batch method's signature has been updated to use readonly tuples, which is a good practice for immutability. Ensure that all usages of this method are updated to match the new signature.
The initial script did not produce any results, which could indicate there are no usages of the batch method or the pattern used was incorrect. I will generate a new script to search for usages of the batch method with a different pattern.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all usages of the `batch` method are updated to match the new signature.
ast-grep --lang typescript --pattern $'batch($_, $$$)'
Length of output: 55
Script:
#!/bin/bash
# Search for usages of the `batch` method in TypeScript files to ensure they match the new signature.
rg 'batch\(' --type ts
Length of output: 1010
SSIA
Summary by CodeRabbit
New Features
Enhancements
Refactor
Bug Fixes
Documentation
Style
Tests
Chores
Revert