💪 Refactoring for denops v6 #292
Conversation
WalkthroughThe changes across the codebase reflect a significant refactoring effort, focusing on simplifying function signatures, updating dependencies, and enhancing type safety. Deprecated functionalities have been removed, and there's a shift towards using promises and type guards. The updates also include performance logging enhancements and a move towards stricter version control in imports. Changes
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? TipsChat with CodeRabbit Bot (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
==========================================
+ Coverage 92.59% 96.42% +3.83%
==========================================
Files 3 3
Lines 81 84 +3
Branches 6 5 -1
==========================================
+ Hits 75 81 +6
+ 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (14)
- autoload/denops/plugin.vim (2 hunks)
- denops/@denops-private/cli.ts (2 hunks)
- denops/@denops-private/error.ts (1 hunks)
- denops/@denops-private/host/base.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (3 hunks)
- denops/@denops-private/host/vim.ts (3 hunks)
- denops/@denops-private/impl.ts (3 hunks)
- denops/@denops-private/impl_test.ts (1 hunks)
- denops/@denops-private/service.ts (5 hunks)
- denops/@denops-private/util.ts (1 hunks)
- denops/@denops-private/version.ts (1 hunks)
- denops/@denops-private/worker/script.ts (3 hunks)
- doc/denops.txt (2 hunks)
Files skipped from review due to trivial changes (3)
- denops/@denops-private/error.ts
- denops/@denops-private/impl_test.ts
- denops/@denops-private/version.ts
Additional comments: 38
autoload/denops/plugin.vim (4)
59-81:
The deprecation logic indenops#plugin#registeris well-implemented, providing clear instructions for users to update their code.84-93:
The deprecation logic indenops#plugin#reloadis well-implemented, providing clear instructions for users to update their code.96-108:
The deprecation logic indenops#plugin#discoveris well-implemented, providing clear instructions for users to update their code.141-147:
Thes:registerfunction correctly implements the simplified plugin registration process.denops/@denops-private/cli.ts (2)
1-6:
The import statements have been updated to the newer versions as described in the summary. Ensure that these updates do not introduce breaking changes and that all dependent code has been updated accordingly.49-52:
TheparseArgsfunction is used correctly with the appropriate options for command line arguments. Ensure that all scripts and documentation that rely on the CLI have been updated to reflect any changes in the available options.denops/@denops-private/host/base.ts (2)
1-3:
The change to importInvokeras a type is appropriate since it's only used for type annotations.18-23:
The changes to thebatchmethod's signature, using readonly tuples for both parameters and return types, enhance type safety and immutability.denops/@denops-private/host/invoker.ts (2)
1-2:
The import statements have been updated correctly to reflect the changes in the summary.11-17:
Theregisterandreloadmethods in theInvokerclass have been updated to accept fewer parameters and return promises, aligning with the summary's description of the changes.denops/@denops-private/host/nvim.ts (2)
74-90:
The refactoring of thebatchfunction to useensurefor result validation and to return a readonly tuple aligns with the summary and enhances type safety. The error handling within the function is also correctly implemented.111-113:
The simplification of thewaitClosedfunction to return a promise directly is consistent with the refactoring goals and aligns with the summary.denops/@denops-private/host/vim.ts (4)
1-8:
The import statements are correctly updated to reflect the new dependencies and type imports.10-20:
The introduction of type predicatesisCallReturnandisBatchReturnenhances type safety for thecallandbatchmethods.37-58:
The refactoring of thecallandbatchmethods to use promises andensurefor error handling aligns with the goal of enhancing type safety and error handling.100-102:
Thedispatchfunction correctly uses the newisInvokeMessagepredicate to ensure the correct message format is being processed.denops/@denops-private/impl.ts (8)
1-4:
The addition ofensureandisfrom the external module aligns with the refactoring goals of enhancing type safety and error handling.16-16:
The introduction ofisBatchReturnis a good practice for ensuring the type safety of batch operation results.42-43:
Theredrawfunction correctly returns a promise directly, aligning with the promise-oriented approach mentioned in the summary.46-47:
Thecallfunction has been updated to return the result of the session call directly, which is consistent with the refactoring goals.50-62:
Thebatchfunction's refactoring to useensurefor result validation and to throw aBatchErrorwhen needed is a good enhancement for error handling.65-66:
Thecmdfunction has been updated to return a promise directly, which is consistent with the changes described in the summary.69-70:
Theevalfunction has been updated to return the result of the session call directly, which is consistent with the refactoring goals.73-79:
Thedispatchfunction has been updated to return the result of the session call directly, aligning with the promise-oriented approach.denops/@denops-private/service.ts (6)
1-21:
The imports have been updated to newer versions, and theensurefunction has been added, which aligns with the summary.48-58:
Theregistermethod has been correctly updated to be asynchronous and themetaandoptionsparameters have been removed as per the summary.68-79:
The use ofperformance.now()as a suffix for the script URL inworker.postMessageis a good practice to avoid caching issues when reloading scripts.83-98:
Thereloadmethod has been correctly updated to be asynchronous and themetaandoptionsparameters have been removed as per the summary.112-136:
Thedisposemethod has been updated to asynchronously dispose of all plugins, which is consistent with the summary.143-173:
The dispatcher methods have been updated, and thetraceparameter has been removed, which is consistent with the summary.denops/@denops-private/util.ts (1)
- 1-9:
The implementation of the
isMetapredicate function correctly validates theMetatype according to the provided summary. The use of theisutility and TypeScript's type system is appropriate and follows best practices for type safety and validation.denops/@denops-private/worker/script.ts (5)
18-20:
The newisMessageDatavalidation object correctly checks for the structure of the incoming message data, ensuring type safety.23-26:
Theemitfunction is a good abstraction for event emission, and the error handling within it is appropriate for avoiding unhandled promise rejections.28-36:
The removal of thetraceparameter from themainfunction is consistent with the summary, and the function does not referencetraceelsewhere.72-84:
The try-catch block in themainfunction correctly handles the plugin lifecycle events and ensures that the worker is closed properly after execution or in case of an error.89-96:
The event listener correctly uses theensurefunction withisMessageDatafor data validation, which is a good practice for type safety and error prevention.doc/denops.txt (2)
84-90:
The summary mentions the removal ofg:denops#trace, but this hunk showsg:denops#debug. Please clarify ifg:denops#debugshould be removed or if the summary should be updated.262-289:
The changes to the documentation of the plugin functions align with the summary provided, reflecting the simplified function signatures and removal of deprecated options.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- autoload/denops/plugin.vim (2 hunks)
Additional comments: 4
autoload/denops/plugin.vim (4)
59-81:
The refactoring ofdenops#plugin#registercorrectly handles the deprecation of the old call patterns and ensures that the function can only be called with the correct number of arguments post-deprecation.84-93:
The refactoring ofdenops#plugin#reloadcorrectly handles the deprecation of theoptionsargument and ensures that no additional arguments are processed.96-108:
The refactoring ofdenops#plugin#discovercorrectly handles the deprecation of theoptionsargument and ensures that no additional arguments are processed.141-147:
The refactoring ofs:registercorrectly updates the function signature to accept only thepluginandscriptarguments and correctly implements the registration logic.
lambdalisue
force-pushed
the
ref
branch
2 times, most recently
from
a52f086 to
f677826
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (16)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/_internal/server/proc.vim (1 hunks)
- autoload/denops/plugin.vim (2 hunks)
- denops/@denops-private/cli.ts (3 hunks)
- denops/@denops-private/error.ts (1 hunks)
- denops/@denops-private/host/base.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (4 hunks)
- denops/@denops-private/host/vim.ts (3 hunks)
- denops/@denops-private/impl.ts (3 hunks)
- denops/@denops-private/impl_test.ts (1 hunks)
- denops/@denops-private/service.ts (5 hunks)
- denops/@denops-private/util.ts (1 hunks)
- denops/@denops-private/version.ts (1 hunks)
- denops/@denops-private/worker/script.ts (3 hunks)
- doc/denops.txt (2 hunks)
Files skipped from review due to trivial changes (2)
- denops/@denops-private/error.ts
- denops/@denops-private/version.ts
Additional comments: 49
autoload/denops/_internal/server/chan.vim (1)
- 93-97:
The change fromdoautocmd User DenopsReadytocall s:rpcnotify(s:chan, 'void', [])is a significant alteration in the event notification mechanism. Ensure that all dependent features and plugins are compatible with this new approach.autoload/denops/_internal/server/proc.vim (1)
- 67-77:
The conditional logic for appending the--quietflag based on theg:denops#debugvariable is correctly implemented and aligns with the summary.autoload/denops/plugin.vim (4)
57-82:
The refactoring ofdenops#plugin#registerfunction correctly handles backward compatibility by providing deprecation warnings and fallbacks for the old function signatures.84-93:
The refactoring ofdenops#plugin#reloadfunction correctly includes a deprecation warning for the removedoptionsargument.96-108:
The refactoring ofdenops#plugin#discoverfunction correctly includes a deprecation warning for the removedoptionsargument and iterates over discovered plugins to register them.141-147:
Thes:registerfunction has been updated to match the new plugin registration process, and it correctly notifies the denops server to invoke the 'register' command with the necessary arguments.denops/@denops-private/cli.ts (3)
1-8:
The import changes align with the summary provided and reflect the updated dependencies and utilities.57-60:
The usage ofparseArgsfor parsing command-line arguments is correct and the options are properly configured.69-75:
The use ofconsole.logfor printing the identity is correct, and the use ofconsole.warnfor the listening address and port is consistent with the other uses ofconsole.warnin the code.denops/@denops-private/host/base.ts (2)
1-3:
The change to importInvokeras a type is appropriate since it's only used for type information.18-23:
The changes to thebatchfunction's parameter and return types enhance type safety and immutability, which are beneficial for the codebase.denops/@denops-private/host/invoker.ts (2)
1-2:
The import statements have been correctly updated to the new versions and paths as per the summary.11-17:
Theregisterandreloadmethods have been correctly updated to accept fewer parameters, aligning with the refactoring goals.denops/@denops-private/host/nvim.ts (7)
1-5:
The import from "unknownutil" has been updated to v3.11.0, which aligns with the pull request summary mentioning the use of the latest version ofensureandisutilities.15-23:
The addition ofisNvimCallFunctionReturnfor type checking aligns with the pull request's focus on enhancing type safety.30-31:
The constructor of theNeovimclass has been modified, and the#invokerproperty has been added to the class, as per the summary.34-58:
The#sessionsetup and dispatcher methods have been refactored to useensurePromisefor handling invoker methods, which is consistent with the pull request's focus on error handling and validation.94-109:
Thebatchmethod signature has been updated to enhance type safety by using readonly tuples, which is in line with the pull request's focus on type safety enhancements.112-114:
Theregistermethod has been updated to assign theinvokerto the#invokerproperty, which is consistent with the summary.129-131:
The addition of theensurePromisefunction to handle both synchronous and asynchronous values aligns with the pull request's focus on error handling and validation.denops/@denops-private/host/vim.ts (7)
10-26:
The introduction of new type guardsisCallReturn,isBatchReturn,isVoidMessage, andisInvokeMessageenhances type safety and ensures that the data conforms to expected types before processing.57-57:
The change to return a resolved Promise from theredrawmethod aligns with the asynchronous nature of the method and ensures consistency with other methods that return a Promise.60-70:
The refactoring of thecallmethod to return a Promise and use theensurefunction for runtime type checking is a good practice, as it improves error handling and ensures that the returned value is of the expected type.72-78:
The update to thebatchmethod to return a Promise with a readonly array as the result is a good practice, as it enforces immutability of the returned data and enhances type safety.80-82:
The update to theregistermethod to assign theinvokerto a class property is a straightforward change that simplifies the method's implementation.84-102:
The introduction of the private method#dispatchcentralizes message processing, which can improve maintainability and make the class easier to understand.113-114:
Thedisposemethod's handling of the session shutdown with a try-catch block that ignores errors is a common pattern for cleanup functions, ensuring that any errors during shutdown do not impact the disposal process.denops/@denops-private/impl.ts (5)
16-16:
The definition ofisBatchReturncorrectly uses theisutility to ensure the structure of the batch return value.46-47:
Thecallfunction correctly returns the result of the session call directly.50-62:
Thebatchfunction correctly handles the session call result, usingensurefor validation and throwing aBatchErrorwhen appropriate.69-70:
Theevalfunction correctly returns the result of the session call directly.73-79:
Thedispatchfunction correctly returns the result of the session call directly.denops/@denops-private/impl_test.ts (2)
1-7:
The import paths have been updated to the newer version of the Deno standard library, which is consistent with the summary.14-30:
The new test step "denops.redraw() does nothing" correctly asserts thatdenops.redraw()returnsundefinedfor different arguments, aligning with the intended behavior.denops/@denops-private/service.ts (8)
60-63:
The implementation of a cache-busting mechanism using a timestamp as a URL fragment is a good practice to ensure that the latest version of the script is loaded, especially when reloading plugins.78-90:
The simplification of thereloadmethod by removing unused parameters and relying on the stored script path is a good refactor for maintainability and clarity.107-109:
UsingPromise.allto dispose of plugins concurrently is a good practice for performance, as it allows for parallel cleanup operations.137-140:
Thereloaddispatcher method correctly asserts the type of thetraceparameter and handles the reload operation without using the deprecatedtraceparameter.143-145:
Theredrawdispatcher method correctly asserts the type of theforceparameter and delegates the redraw operation to the host, which is a clean and type-safe implementation.148-151:
Thecalldispatcher method correctly asserts the types of thefnandargsparameters and delegates the call operation to the host, ensuring type safety and proper delegation.154-156:
Thebatchdispatcher method correctly asserts the type of thecallsparameter and delegates the batch operation to the host, which is a good practice for type safety and code clarity.159-163:
Thedispatchdispatcher method correctly asserts the types of thename,fn, andargsparameters and handles the dispatch operation within the service, ensuring type safety and proper internal delegation.denops/@denops-private/util.ts (1)
- 4-9:
TheisMetapredicate function correctly validates theMetatype properties using theisutility. The use ofas constfor literal types is appropriate, and the code is modular and maintainable.denops/@denops-private/worker/script.ts (4)
1-14:
The import ofisMetais still present, which is correct as it is being used in theisMessageDataobject definition. No action is needed here.18-20:
TheisMessageDataobject definition correctly usesisMetato validate themetaproperty, ensuring the data structure is as expected.72-84:
Themainfunction has been updated to include proper error handling and event emission, which aligns with the summary of changes.89-96:
The event listener for the worker correctly usesensureandisMessageDatato validate the incoming message data, which is in line with the summary of changes.doc/denops.txt (2)
84-90:
The hunk correctly reflects the removal ofg:denops#traceand the presence ofg:denops#debug.262-288:
The hunk correctly reflects the updated function signatures fordenops#plugin#discover,denops#plugin#register, anddenops#plugin#reloadfunctions, aligning with the summary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (17)
- autoload/denops.vim (1 hunks)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/_internal/server/proc.vim (1 hunks)
- autoload/denops/plugin.vim (2 hunks)
- denops/@denops-private/cli.ts (3 hunks)
- denops/@denops-private/error.ts (1 hunks)
- denops/@denops-private/host/base.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (4 hunks)
- denops/@denops-private/host/vim.ts (3 hunks)
- denops/@denops-private/impl.ts (3 hunks)
- denops/@denops-private/impl_test.ts (1 hunks)
- denops/@denops-private/service.ts (5 hunks)
- denops/@denops-private/util.ts (1 hunks)
- denops/@denops-private/version.ts (1 hunks)
- denops/@denops-private/worker/script.ts (3 hunks)
- doc/denops.txt (2 hunks)
Files skipped from review due to trivial changes (3)
- autoload/denops.vim
- denops/@denops-private/error.ts
- denops/@denops-private/version.ts
Additional comments: 52
autoload/denops/_internal/server/chan.vim (1)
- 95-96: The change from triggering an autocmd event to invoking an RPC notification may affect plugins or parts of the codebase that rely on the
DenopsReadyevent. Ensure that all dependent code has been updated to handle this new notification mechanism.autoload/denops/_internal/server/proc.vim (1)
- 67-78: The conditional addition of the '--quiet' flag based on the
g:denops#debugvariable is a good approach to control the verbosity of the output. Ensure thatg:denops#debugis defined before this code is executed to avoid any runtime errors.autoload/denops/plugin.vim (4)
59-81: The deprecation warnings and conditional logic for handling different argument combinations in
denops#plugin#registerare correctly implemented.84-93: The deprecation warning and the updated call to
denops#server#notifyindenops#plugin#reloadare correctly implemented.96-108: The deprecation warning in
denops#plugin#discoverand the loop that registers discovered plugins are correctly implemented.141-147: The
s:registerfunction has been correctly updated to match the new function signature that accepts only two arguments.denops/@denops-private/cli.ts (4)
1-8: The imports have been updated to use newer versions of dependencies and additional utilities like
ensureandisMeta. Ensure that these new utilities are used correctly throughout the codebase.26-60: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [26-75]
The use of
console.warnfor logging connection information and performance measurements is unconventional but may be intentional to highlight these messages. Ensure that this aligns with the intended logging strategy.
57-60: The
parseArgsfunction is used to parse command-line arguments, and the parsed arguments are used to set up the listener and control logging verbosity. Ensure that the arguments are handled correctly in all relevant parts of the codebase.69-74: The
console.logstatement is used to output the local address when theidentityflag is set, andconsole.warnis used to log the listening address. Ensure that this is consistent with the intended output and logging strategy.denops/@denops-private/host/base.ts (3)
1-2: The change to import
Invokeras a type is appropriate since it seems to be used only for type annotations within this file.21-23: The use of readonly tuples for the
callsparameter in thebatchfunction is a good practice as it enforces immutability, ensuring that the input cannot be modified.23-23: The update to the return type of the
batchfunction to use readonly tuples is consistent with the immutability enforced on thecallsparameter, ensuring that the returned array elements cannot be modified.denops/@denops-private/host/invoker.ts (2)
1-1: The import statement for the
ismodule has been updated to version 3.11.0. Ensure that this version is compatible with the rest of the codebase and that all uses of theismodule have been tested with the new version.11-17: The
registerandreloadmethods have been simplified to accept fewer parameters. Ensure that all calls to these methods have been updated accordingly and that the removed parameters (meta,options, andtrace) are no longer required or have been handled differently in the codebase.denops/@denops-private/host/nvim.ts (7)
3-3: The addition of the
ensureimport is appropriate as it is used within thebatchmethod to validate the return type.28-28: The
#invokerproperty is correctly declared as optional since it is assigned later in theregistermethod.97-108: The
batchmethod has been updated to return a tuple containing the results array and an error string. Ensure that all usages of this method handle the new return type correctly.112-113: The
registermethod correctly assigns the passedinvokerto the#invokerproperty. Ensure that theInvokeris registered before any invocation occurs.116-117: The
waitClosedmethod now returns a promise from theSession'swaitmethod, which is the expected behavior for an asynchronous operation.94-120: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [120-125]
The
disposemethod correctly handles the session shutdown within a try-catch block, ignoring any errors that occur during shutdown.
- 129-131: The
ensurePromiseutility function is correctly implemented to wrap non-promise values in a promise, which is useful for ensuring consistent asynchronous behavior.denops/@denops-private/host/vim.ts (5)
1-1: The import of
ensureandisfrom an external module is appropriate for the new type checks introduced in thecallandbatchmethods.60-70: The
callmethod has been correctly updated to return aPromiseand includes proper error handling.72-78: The
batchmethod has been correctly updated to return aPromisewith a readonly array as the result, which aligns with the new API contract.80-82: The
registermethod has been updated to assign theinvokerto a class property, which is a straightforward and correct change.113-114: The
disposemethod is correctly implemented to handle exceptions silently, which is appropriate for a cleanup function.denops/@denops-private/impl.ts (6)
1-4: The addition of the import statement for
ensureandisis appropriate for the new functionality introduced in thebatchfunction.16-16: The introduction of the
isBatchReturnconstant is a good practice for type validation in thebatchfunction.42-43: The use of
.then()without a callback in theredrawfunction is redundant and can be removed for clarity.- return this.#session.call("redraw", force).then(); + return this.#session.call("redraw", force);
- 65-66: The use of
.then()without a callback in thecmdfunction is redundant and can be removed for clarity.- return this.#session.call("call", "denops#api#cmd", cmd, ctx).then(); + return this.#session.call("call", "denops#api#cmd", cmd, ctx);
50-62: The changes to the
batchfunction, including the use ofensurefor type validation and throwing aBatchError, are correctly implemented.69-79: The modifications to the
evalanddispatchfunctions to return the result of the session call directly are consistent with the refactoring pattern applied to other functions in this file.denops/@denops-private/impl_test.ts (3)
1-6: The version numbers for the dependencies have been updated correctly.
13-30: The new test cases for
denops.redraw()are well-structured and check for the expectedundefinedreturn value for different parameter scenarios.33-35: No changes detected in the remaining parts of the test file outside the hunk.
denops/@denops-private/service.ts (7)
1-10: The dependencies have been updated to newer versions. Ensure that the updated versions are backward compatible and do not introduce breaking changes.
34-38: The
Serviceclass constructor now requires ametaparameter. Verify that all instantiations of theServiceclass have been updated accordingly.41-50: The
registermethod has been simplified to only requirenameandscriptparameters. Ensure that all calls to this method have been updated to match the new signature.78-90: The
reloadmethod has been simplified to only accept thenameparameter. Verify that all calls to this method have been updated to match the new signature.107-109: The
disposemethod now usesdisposePluginfor disposing of plugins. Ensure thatdisposePluginhandles all necessary cleanup tasks.136-140: The
reloaddispatcher method has been updated to no longer accept atraceparameter. Ensure that all dispatch calls toreloadhave been updated to match the new signature.60-63: The
resolveScriptUrlfunction has been updated. Verify that the new implementation correctly resolves script URLs in all scenarios where it is used.denops/@denops-private/util.ts (1)
- 1-9: The implementation of
isMetaand the import statements are correct and follow best practices for type safety and maintainability.denops/@denops-private/worker/script.ts (5)
1-14: The imports have been updated to the latest version, and unused imports have been removed, which is good for maintaining up-to-date dependencies and clean code.
23-26: The new
emitfunction is implemented correctly with proper error handling.28-36: The
mainfunction signature has been updated by removing thetraceparameter, and the function has been updated accordingly.74-77: The
mainfunction now includes calls to the newemitfunction, which seems to be used correctly to emit events before and after the plugin's main function is called.91-96: The event listener has been updated to use the
ensurefunction withisMessageDatafor validation, which is a good practice for type safety and ensures that the event data matches the expected schema.doc/denops.txt (3)
265-272: The addition of
denops#plugin#discover()function is consistent with the summary and provides a way to automatically discover and register denops plugins. This should improve the usability and maintainability of the plugin system.273-282: The simplification of the
denops#plugin#register()function signature by removing the optionalscriptandoptionsparameters is consistent with the summary and should make the plugin registration process more straightforward.284-287: The removal of the
optionsparameter from thedenops#plugin#reload()function is consistent with the summary and simplifies the reloading process.
Eliminated because the disadvantages of this feature are greater than the advantages it brings, and its presence is an obstacle to refactoring.
This is a minor but a breaking change. This change affects the `g:denops#debug` variable. After this commit, the variable must be configured prior to the denops initialization.
Now denops always skip registration or reload on invalid plugins.
We must use `console.warn()` instead of `console.log()` because stdout is used to tell proper address to the host.
Otherwise denops may not be ready on `DenopsReady`.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (17)
- autoload/denops.vim (1 hunks)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/_internal/server/proc.vim (1 hunks)
- autoload/denops/plugin.vim (2 hunks)
- denops/@denops-private/cli.ts (3 hunks)
- denops/@denops-private/error.ts (1 hunks)
- denops/@denops-private/host/base.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (4 hunks)
- denops/@denops-private/host/vim.ts (3 hunks)
- denops/@denops-private/impl.ts (3 hunks)
- denops/@denops-private/impl_test.ts (1 hunks)
- denops/@denops-private/service.ts (5 hunks)
- denops/@denops-private/util.ts (1 hunks)
- denops/@denops-private/version.ts (1 hunks)
- denops/@denops-private/worker/script.ts (3 hunks)
- doc/denops.txt (2 hunks)
Additional comments: 39
denops/@denops-private/util.ts (2)
1-2: The update to the import statements, including the new version of
unknownutil, is correct.4-9: The
isMetadefinition correctly usesis.ObjectOfto validate theMetatype with specific properties.denops/@denops-private/error.ts (1)
- 1-1: The update to the import statement for the
isfunction with the new version ofunknownutilis correct.denops/@denops-private/host/base.ts (2)
1-2: The update to the import statements for
DisposableandInvokerwith type declarations and new versions is correct.21-23: The
batchmethod's signature has been correctly updated to usereadonlyfor its parameters and return type, which is a good practice for immutability.denops/@denops-private/version.ts (1)
- 4-5: The update to the import statements for the
pathandsemvermodules with the new versions is correct.autoload/denops.vim (1)
- 21-26: The removal of the
denops#traceconfiguration definition is consistent with the PR objectives and streamlines the configuration.denops/@denops-private/host/invoker.ts (3)
1-2: The update to the import statements for
unknownutiland the type import forServicewith the new version is correct.11-12: The simplification of the
registermethod by removing unnecessary parameters aligns with the PR objectives to streamline the API.15-16: The simplification of the
reloadmethod by removing unnecessary parameters aligns with the PR objectives to streamline the API.denops/@denops-private/impl.ts (7)
1-1: The addition of the import statement for
ensureandisfrom the updated version ofunknownutilis correct.42-43: The
redrawmethod has been updated to return a promise directly, which is a more modern and concise approach.46-47: The
callmethod has been updated to return a promise directly, which is a more modern and concise approach.50-62: The
batchmethod has been updated to return a promise directly, which is a more modern and concise approach.65-66: The
cmdmethod has been updated to return a promise directly, which is a more modern and concise approach.69-70: The
evalmethod has been updated to return a promise directly, which is a more modern and concise approach.73-78: The
dispatchmethod has been updated to return a promise directly, which is a more modern and concise approach.denops/@denops-private/cli.ts (2)
1-8: The update to the import statements with new versions and additional imports is correct.
26-60: The
handleConnfunction has been updated to include additional logic for detecting hosts and handling services, which aligns with the PR objectives to enhance performance logging.denops/@denops-private/worker/script.ts (2)
1-14: The update to the import statements with new versions and the removal of unused imports is correct.
18-27: The refactoring of the
mainfunction to useisMessageDataand theemitfunction for emitting events aligns with the PR objectives to streamline event handling.denops/@denops-private/host/vim.ts (1)
- 1-26: The addition of type checks using
isfromunknownutiland the new#dispatchmethod enhance type safety and encapsulation of message handling.denops/@denops-private/host/nvim.ts (2)
1-27: The addition of type checks using
isfromunknownutiland the new#invokerproperty enhance type safety and encapsulation of invoker handling.129-131: The
ensurePromisefunction is a good addition to ensure that a value is always treated as a promise, improving code robustness.autoload/denops/_internal/server/chan.vim (1)
- 96-96: Verify that the change from
doautocmdtos:rpcnotifydoes not affect any plugins or event listeners that rely on theDenopsReadyevent.autoload/denops/_internal/server/proc.vim (1)
- 73-74: The conditional addition of the
--quietflag based on theg:denops#debugvariable is a good practice to control verbosity based on the debug setting.denops/@denops-private/service.ts (4)
30-38: The addition of the
metaproperty to theServiceclass constructor is a good practice for passing metadata, enhancing the class's functionality.41-50: The simplification of the
registermethod by removing themetaandtraceparameters aligns with the removal of theg:denops#traceconfiguration and streamlines the plugin registration process.78-90: The changes to the
reloadmethod, which now accepts only thenameparameter, simplify the code and align with the refactoring objectives.136-166: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [121-166]
The simplification of the
buildServiceSessionfunction by removing conditional trace logic aligns with the removal of theg:denops#traceconfiguration and improves code maintainability.denops/@denops-private/impl_test.ts (1)
- 13-31: The new test case for
denops.redraw()is well-structured and follows best practices by checking the function's behavior with different arguments. It's good to see multiple assertions covering different scenarios.autoload/denops/plugin.vim (4)
59-81: The deprecation warnings and argument handling in
denops#plugin#registerare correctly implemented. The function now guides users to the new expected usage, which is a good practice for backward compatibility.84-93: The deprecation warnings in
denops#plugin#reloadare correctly implemented, and the function now ignores any additional arguments, aligning with the updated function signature.96-107: The
denops#plugin#discoverfunction has been updated to remove the handling of options, and it now automatically discovers and registers plugins. This change simplifies the plugin discovery process.141-147: The
s:registerfunction has been updated to match the new plugin registration process, ensuring consistency across the codebase.doc/denops.txt (4)
90-95: The documentation update for
g:denops#debugcorrectly reflects the changes in the codebase and provides clear information to the users about the debug mode.271-277: The documentation for the new
denops#plugin#discoverfunction is clear and accurately describes its purpose and behavior.279-283: The updates to the documentation for
denops#plugin#registercorrectly reflect the changes to the function's behavior and signature, providing clarity on its usage.290-293: The documentation for
denops#plugin#reloadhas been simplified to match the updated behavior of the function, which is now more efficient in handling plugin reloading.
| this.#invoker = invoker; | ||
| } | ||
|
|
||
| async #dispatch(expr: unknown): Promise<unknown> { |
There was a problem hiding this comment.
The #dispatch method is marked as async but does not contain any await expressions, which could be removed for simplification.
- async #dispatch(expr: unknown): Promise<unknown> {
+ #dispatch(expr: unknown): Promise<unknown> {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.
| async #dispatch(expr: unknown): Promise<unknown> { | |
| #dispatch(expr: unknown): Promise<unknown> { |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- autoload/denops/_internal/job.vim (1 hunks)
- autoload/denops/_internal/path.vim (2 hunks)
- autoload/denops/_internal/server/chan.vim (3 hunks)
- autoload/denops/_internal/server/proc.vim (2 hunks)
- autoload/denops/_internal/wait.vim (1 hunks)
- autoload/denops/server.vim (1 hunks)
- autoload/health/denops.vim (5 hunks)
Files skipped from review due to trivial changes (2)
- autoload/denops/_internal/job.vim
- autoload/denops/server.vim
Files skipped from review as they are similar to previous changes (2)
- autoload/denops/_internal/server/chan.vim
- autoload/denops/_internal/server/proc.vim
Additional comments: 12
autoload/denops/_internal/path.vim (4)
1-2: The change from mutable variables to immutable constants for path separators and root directory is a good practice for values that should not change during runtime.
5-5: The update to use
s:SEPin thedenops#_internal#path#joinfunction correctly reflects the refactoring to constants.22-22: The use of
s:ROOTands:SEPin thes:scriptfunction for Windows maintains consistency with the refactoring to constants.30-30: The use of
s:ROOTin thes:scriptfunction for non-Windows systems is consistent with the refactoring to constants.autoload/denops/_internal/wait.vim (1)
- 23-41: The refactoring of the
s:waitfunction to usegetcharstr(0)and manage consumed input withfeedkeysis a significant improvement for handling user input during the waiting process. Ensure that this change has been tested thoroughly, especially the behavior offeedkeyswith different modes.autoload/health/denops.vim (7)
1-3: Changing version declarations to constants is a good practice to prevent accidental modification and to indicate that these values are fixed.
49-49: The use of
s:DENO_VERSIONin thes:check_deno_versionfunction correctly reflects the refactoring to constants.58-61: The comparison logic in
s:check_deno_versionusings:DENO_VERSIONis correct and aligns with the refactoring to constants.71-71: The use of
s:VIM_VERSIONin thes:check_vim_versionfunction correctly reflects the refactoring to constants.77-80: The comparison logic in
s:check_vim_versionusings:VIM_VERSIONis correct and aligns with the refactoring to constants.90-90: The use of
s:NEOVIM_VERSIONin thes:check_neovim_versionfunction correctly reflects the refactoring to constants.96-99: The comparison logic in
s:check_neovim_versionusings:NEOVIM_VERSIONis correct and aligns with the refactoring to constants.
Make it happen!
Summary by CodeRabbit
New Features
denops#plugin#discover()function to automatically discover and register plugins.Enhancements
denops#plugin#register()function by simplifying its signature.denops#plugin#reload()function to improve performance and reliability.Bug Fixes
Refactor
Documentation
g:denops#tracevariable and the addition of new plugin discovery functionality.Chores