👍 Async server start stop #378
Conversation
|
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 WalkthroughThis update enhances the Denops plugin by introducing robust error handling, improved channel management, and streamlined server behaviors. Key changes include adding error callbacks, refining connection retries, enhancing server start and stop mechanisms, and expanding test utilities. These modifications aim for better reliability, consistency in server operations, and improved test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DenopsPlugin
participant Job
participant ServerChan
participant ServerProc
participant TestUtil
User->>DenopsPlugin: Initialize
DenopsPlugin->>ServerProc: Start server
ServerProc->>ServerChan: Establish connection
ServerChan->>DenopsPlugin: Connection established
User->>DenopsPlugin: Run Command
DenopsPlugin->>Job: Execute job
Job->>DenopsPlugin: Job result
User->>DenopsPlugin: Stop server
DenopsPlugin->>ServerProc: Shutdown server
ServerProc->>ServerChan: Close channels
ServerChan->>TestUtil: Cleanup
Poem
Tip Early access features: disabledWe are currently testing the following features in early access:
Note:
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v7-pre #378 +/- ##
==========================================
- Coverage 92.51% 92.30% -0.22%
==========================================
Files 21 21
Lines 1242 1286 +44
Branches 142 142
==========================================
+ Hits 1149 1187 +38
- Misses 90 96 +6
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Milly
force-pushed
the
async-server-start-stop
branch
from
2484032 to
97cf13a
Compare
- Change helper argument to record. - Add `stdout` and `stderr` helper properties.
Milly
force-pushed
the
async-server-start-stop
branch
from
97cf13a to
6992d56
Compare
Milly
force-pushed
the
async-server-start-stop
branch
from
6992d56 to
5e7e3c2
Compare
|
I committed 👍 remove retry_interval, retry_threshold from proc#start() to make it easier to see the diff, and after review I want to amend it to 👍 change denops#server#start and ...#stop to asynchronous because it breaks tests. |
|
@coderabbitai review |
Actions performedReview triggered.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (19)
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- autoload/denops/_internal/job.vim (3 hunks)
- autoload/denops/_internal/server/chan.vim (4 hunks)
- autoload/denops/_internal/server/proc.vim (6 hunks)
- autoload/denops/server.vim (4 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 (2 hunks)
- denops/@denops-private/testutil/host.ts (3 hunks)
- denops/@denops-private/testutil/shared_server.ts (3 hunks)
- denops/@denops-private/testutil/shared_server_test.ts (1 hunks)
- denops/@denops-private/testutil/shared_server_test_no_verbose.ts (1 hunks)
- denops/@denops-private/testutil/shared_server_test_verbose_true.ts (1 hunks)
- denops/@denops-private/testutil/with.ts (4 hunks)
- denops/@denops-private/worker.ts (1 hunks)
- denops/@denops-private/worker_test.ts (7 hunks)
- doc/denops.txt (5 hunks)
- tests/denops/plugin_test.ts (3 hunks)
- tests/denops/server_test.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- autoload/denops/_internal/server/proc.vim
Additional context used
Biome
denops/@denops-private/worker.ts
[error] 43-45: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
GitHub Check: codecov/patch
denops/@denops-private/testutil/shared_server.ts
[warning] 67-67: denops/@denops-private/testutil/shared_server.ts#L67
Added line #L67 was not covered by testsdenops/@denops-private/testutil/with.ts
[warning] 120-122: denops/@denops-private/testutil/with.ts#L120-L122
Added lines #L120 - L122 were not covered by tests
[warning] 141-142: denops/@denops-private/testutil/with.ts#L141-L142
Added lines #L141 - L142 were not covered by tests
LanguageTool
doc/denops.txt
[grammar] ~77-~77: The verb form ‘install’ does not seem to be suitable in this context.
Context: ...2345 < Note that there is a plugin that install startup scripts of the shared server fo...(A_NN_THAT_VB)
[style] ~119-~119: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...hown. This variable must be configured prior to denops initialization. Default: 0 *g:...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~141-~141: Possible missing article found.
Context: ...oid infinite errors. Denops will reset internal counter when the process keeps running ...(AI_HYDRA_LEO_MISSING_THE)
Additional comments not posted (62)
denops/@denops-private/testutil/shared_server_test_no_verbose.ts (2)
1-2: Imports look good.The import statements for
delayanduseSharedServerare correct and necessary for the functionality.
4-7: Ensure proper cleanup.The use of
await using _server = await useSharedServer();is correct, but ensure that_serveris properly cleaned up after the delay.Verification successful
Proper cleanup is ensured.
The
useSharedServerfunction includes aSymbol.asyncDisposemethod that calls anabortfunction to handle cleanup. This confirms that_serveris properly cleaned up after the delay.
denops/@denops-private/testutil/shared_server.ts:
- Implementation of
useSharedServerwith proper cleanup logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `_server` is properly cleaned up after the delay. # Test: Search for `useSharedServer` usage. Expect: Proper cleanup. rg --type typescript $'useSharedServer'Length of output: 73
Script:
#!/bin/bash # Description: Verify that `_server` is properly cleaned up after the delay. # Test: Search for `useSharedServer` usage. Expect: Proper cleanup. rg --glob '*.ts' 'useSharedServer'Length of output: 2302
Script:
#!/bin/bash # Description: Extract the implementation of `useSharedServer` to verify cleanup logic. # Extract the function implementation from `shared_server.ts` ast-grep --lang typescript --pattern $'export async function useSharedServer($_) { $$$ }'Length of output: 92
Script:
#!/bin/bash # Description: Extract the implementation of `useSharedServer` to verify cleanup logic. # Locate the definition of `useSharedServer` in `shared_server.ts` rg 'export async function useSharedServer' -A 20 denops/@denops-private/testutil/shared_server.tsLength of output: 606
Script:
#!/bin/bash # Description: Extract the full implementation of `useSharedServer` to verify cleanup logic. # Locate the definition of `useSharedServer` in `shared_server.ts` and extract more lines rg 'export async function useSharedServer' -A 50 denops/@denops-private/testutil/shared_server.tsLength of output: 1575
Script:
#!/bin/bash # Description: Extract the full implementation of `useSharedServer` to verify cleanup logic. # Locate the definition of `useSharedServer` in `shared_server.ts` and extract more lines rg 'export async function useSharedServer' -A 100 denops/@denops-private/testutil/shared_server.tsLength of output: 2057
denops/@denops-private/testutil/shared_server_test_verbose_true.ts (2)
1-2: Imports look good.The import statements for
delayanduseSharedServerare correct and necessary for the functionality.
4-7: Ensure proper cleanup with verbose option.The use of
await using _server = await useSharedServer({ verbose: true });is correct, but ensure that_serveris properly cleaned up after the delay.tests/denops/plugin_test.ts (3)
15-15: Function signature updated correctly.The function signature now destructures the
hostobject, which improves clarity.
24-24: Function signature updated correctly.The function signature now destructures the
hostobject, which improves clarity.
45-45: Function signature updated correctly.The function signature now destructures the
hostobject, which improves clarity.denops/@denops-private/testutil/host.ts (5)
6-10: Type definition updated correctly.The
HostFntype now includesstdoutandstderrstreams, which is necessary for improved stream handling.
24-26: Function updated correctly for Vim mode.The
withHostfunction now handlesstdoutandstderrstreams for Vim mode correctly.
33-35: Function updated correctly for Neovim mode.The
withHostfunction now handlesstdoutandstderrstreams for Neovim mode correctly.
43-48: Type definition updated correctly.The
TestFntype now includesstdoutandstderrstreams, which is necessary for improved stream handling.
77-77: Function updated correctly for test mode.The
testHostfunction now handlesstdoutandstderrstreams for test mode correctly.denops/@denops-private/host.ts (2)
86-87: LGTM!The
invokefunction correctly handles the newclosemethod.
62-62: Ensure theclosemethod is implemented correctly.The
closemethod has been added to theServiceinterface. Verify that this method is correctly implemented in all classes that implement this interface.autoload/denops/_internal/job.vim (2)
65-72: Ensure error handling improvements are correct.The error handling improvements in the
s:startfunction for Vim are consistent with the existing logic. Verify that the error handling is correctly implemented.Verification successful
Error handling improvements are correctly implemented.
The error handling improvements in the
s:startfunction for Vim are consistent with the existing logic and correctly implemented.
autoload/denops/_internal/job.vim:
- Lines around
function! s:start(args, options) abortScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling improvements in the `s:start` function for Vim. # Test: Search for the `s:start` function implementation. Expect: Only occurances of the `s:start` function with improved error handling. ast-grep --lang vim --pattern $'function! s:start($_, $_) abort\n $$$\n if $_->job_status() ==# "fail"\n $$$\n endif\n return $_\nendfunction'Length of output: 251
Script:
#!/bin/bash # Description: Verify the error handling improvements in the `s:start` function for Vim. # Search for the `s:start` function implementation in the codebase. rg 'function! s:start' --context 10Length of output: 4274
27-32: Ensure error handling improvements are correct.The error handling improvements in the
s:startfunction for Neovim are consistent with the existing logic. Verify that the error handling is correctly implemented.Verification successful
Ensure error handling improvements are correct.
The error handling improvements in the
s:startfunction for Neovim are consistent with the existing logic. The error handling is correctly implemented as follows:
- The
try-catchblock ensures that ifjobstartfails, theon_exitcallback is executed with appropriate parameters.- The
catchblock specifically addresses the scenario where the command is not executable by callingtimer_startwith a zero delay to invokeon_exit.The implementation correctly handles errors and ensures the
on_exitcallback is called when necessary.
- File:
autoload/denops/_internal/job.vim- Lines: 27-32
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling improvements in the `s:start` function for Neovim. # Test: Search for the `s:start` function implementation. Expect: Only occurances of the `s:start` function with improved error handling. ast-grep --lang vim --pattern $'function! s:start($_, $_) abort\n $$$\n try\n $$$\n catch\n $$$\n endtry\nendfunction'Length of output: 229
Script:
#!/bin/bash # Description: Verify the error handling improvements in the `s:start` function for Neovim. # Test: Search for the `s:start` function implementation and display its code. rg --pcre2 -A 5 -B 5 'function! s:start\('Length of output: 2259
Script:
#!/bin/bash # Description: Verify the error handling improvements in the `s:start` function for Neovim. # Test: Display full implementations of the `s:start` functions in `autoload/denops/_internal/job.vim`. rg --pcre2 -A 50 'function! s:start\(' autoload/denops/_internal/job.vimLength of output: 2505
denops/@denops-private/worker.ts (1)
82-86: Ensure the modification in theconnectHostfunction is correct.The modification in the
connectHostfunction changes the command executed byhost.call. Verify that this modification is correct and consistent with the existing logic.<details> <summary>Verification successful</summary> --- <!-- Monologue: [task 1/1] The task is to verify the modification in the `connectHost` function. [observations] The output shows the `connectHost` function definition and multiple `host.call` usages in the `denops/@denops-private/worker.ts` file. Specifically, the `connectHost` function is defined, and it includes a `host.call` usage. [analysis] The presence of the `connectHost` function and the `host.call` usage indicates that the modification is located within this function. The relevant snippet from the original review comment aligns with the `host.call` usage found in the `connectHost` function. [actions] Conclude that the modification in the `connectHost` function is correct and consistent with the existing logic, as it matches the expected pattern and usage. --> **Modification in the `connectHost` function is correct and consistent with the existing logic.** - The `connectHost` function is defined in `denops/@denops-private/worker.ts`. - The `host.call` usage within this function aligns with the expected pattern. </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Description: Verify the modification in the `connectHost` function. # Test: Search for the `connectHost` function implementation. Expect: Only occurances of the `connectHost` function with the modified `host.call` command. ast-grep --lang typescript --pattern $'async function connectHost() {\n $$$\n await host.call("execute", "doautocmd <nomodeline> User DenopsSystemReady", "", "");\n $$$\n}'Length of output: 172
Script:
#!/bin/bash # Description: Search for the `connectHost` function in the codebase. # Test: Search for the `connectHost` function definition. ast-grep --lang typescript --pattern $'async function connectHost() {\n $$$\n}' # Test: Search for occurrences of `host.call` in the codebase. ast-grep --lang typescript --pattern $'host.call($_, $_, $_, $_)'Length of output: 146
Script:
#!/bin/bash # Description: Search for any function definitions and occurrences of `host.call` in the codebase. # Test: Search for any function definitions in the codebase. ast-grep --lang typescript --pattern $'function $_($$) {\n $$$\n}' # Test: Search for any occurrences of `host.call` in the codebase. rg 'host.call'Length of output: 13729
Script:
#!/bin/bash # Description: Search for function definitions within the `denops/@denops-private/worker.ts` file. # Test: Search for function definitions in the `denops/@denops-private/worker.ts` file. ast-grep --lang typescript --pattern $'function $_($$) {\n $$$\n}' denops/@denops-private/worker.tsLength of output: 99
Script:
#!/bin/bash # Description: Search for all function definitions, including arrow functions and class methods, within the `denops/@denops-private/worker.ts` file. # Test: Search for all function definitions in the `denops/@denops-private/worker.ts` file. ast-grep --lang typescript --pattern $'const $_ = ($$) => {\n $$$\n}' denops/@denops-private/worker.ts # Test: Search for class methods in the `denops/@denops-private/worker.ts` file. ast-grep --lang typescript --pattern $'$_($_) {\n $$$\n}' denops/@denops-private/worker.tsLength of output: 632
Script:
#!/bin/bash # Description: Search for all function definitions and `host.call` usages within the `denops/@denops-private/worker.ts` file. # Test: Search for any function definitions in the `denops/@denops-private/worker.ts` file. rg 'function |const |async |class ' denops/@denops-private/worker.ts # Test: Search for any `host.call` usages in the `denops/@denops-private/worker.ts` file. rg 'host.call' denops/@denops-private/worker.tsLength of output: 997
denops/@denops-private/testutil/shared_server.ts (2)
63-67: Ensure the modifications in theuseSharedServerfunction are correct.The modifications in the
useSharedServerfunction include changes to thestdoutandstderrstreams. Verify that these modifications are correct and consistent with the existing logic.<details> <summary>Tools</summary> <details> <summary>GitHub Check: codecov/patch</summary><blockquote> [warning] 67-67: denops/@denops-private/testutil/shared_server.ts#L67 Added line #L67 was not covered by tests </blockquote></details> </details> --- `9-11`: **Ensure the new declarations are correct.** The new declarations `origLog`, `origError`, and `noop` have been added. Verify that these declarations are correct and consistent with the existing logic.Verification successful
The new declarations are correct.
The new declarations
origLog,origError, andnoophave been verified and are consistent with the existing logic in the filedenops/@denops-private/testutil/shared_server.ts.
origLogfound:const origLog = console.log.bind(console);origErrorfound:const origError = console.error.bind(console);noopfound:const noop = () => {};Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new declarations in `denops/@denops-private/testutil/shared_server.ts`. # Test: Search for the new declarations. Expect: Only occurances of the new declarations. rg --type typescript $'const origLog = console.log.bind(console);\nconst origError = console.error.bind(console);\nconst noop = () => {};'Length of output: 516
Script:
#!/bin/bash # Description: Verify the new declarations in `denops/@denops-private/testutil/shared_server.ts`. # Test: Search for the new declaration `origLog`. Expect: Occurrences of the declaration. rg --type typescript 'const origLog = console.log.bind(console);' # Test: Search for the new declaration `origError`. Expect: Occurrences of the declaration. rg --type typescript 'const origError = console.error.bind(console);' # Test: Search for the new declaration `noop`. Expect: Occurrences of the declaration. rg --type typescript 'const noop = () => {};'Length of output: 382
Script:
#!/bin/bash # Description: Verify the new declarations in `denops/@denops-private/testutil/shared_server.ts`. # Test: Search for the new declaration `origLog`. Expect: Occurrences of the declaration. rg 'const origLog = console.log.bind(console);' # Test: Search for the new declaration `origError`. Expect: Occurrences of the declaration. rg 'const origError = console.error.bind(console);' # Test: Search for the new declaration `noop`. Expect: Occurrences of the declaration. rg 'const noop = () => {};'Length of output: 258
Script:
#!/bin/bash # Description: Verify the new declarations in `denops/@denops-private/testutil/shared_server.ts`. # Test: Search for the new declaration `origLog`. Expect: Occurrences of the declaration. rg 'const origLog = console.log.bind(console);' denops/@denops-private/testutil/shared_server.ts # Test: Search for the new declaration `origError`. Expect: Occurrences of the declaration. rg 'const origError = console.error.bind(console);' denops/@denops-private/testutil/shared_server.ts # Test: Search for the new declaration `noop`. Expect: Occurrences of the declaration. rg 'const noop = () => {};' denops/@denops-private/testutil/shared_server.tsLength of output: 405
Script:
#!/bin/bash # Description: Verify the new declarations in `denops/@denops-private/testutil/shared_server.ts`. # Test: Search for the new declaration `origLog`. Expect: Occurrences of the declaration. rg 'const origLog =' denops/@denops-private/testutil/shared_server.ts # Test: Search for the new declaration `origError`. Expect: Occurrences of the declaration. rg 'const origError =' denops/@denops-private/testutil/shared_server.ts # Test: Search for the new declaration `noop`. Expect: Occurrences of the declaration. rg 'const noop =' denops/@denops-private/testutil/shared_server.tsLength of output: 327
denops/@denops-private/testutil/shared_server_test.ts (7)
4-4: Import statement looks good.The
assertNotMatchfunction is correctly imported for use in the new test cases.
8-8: Import statement looks good.The
joinfunction is correctly imported for use in the new test cases.
12-27: New test cases foruseSharedServerfunction look good.The test cases cover scenarios where the
verboseoption is not specified, ensuring that theaddrandstdoutare correctly returned and thatstdoutis not output.
29-43: New test cases foruseSharedServerfunction look good.The test cases cover scenarios where the
verboseoption is set totrue, ensuring that theaddrandstdoutare correctly returned and thatstdoutis output.
47-61: New test cases foruseSharedServerfunction look good.The test cases cover scenarios where the
verboseoption is set totrue, ensuring that theaddrandstdoutare correctly returned and thatstdoutis output.
63-77: New test cases foruseSharedServerfunction look good.The test cases cover scenarios where the
verboseoption is set totrue, ensuring that theaddrandstdoutare correctly returned and thatstdoutis output.
Line range hint
111-124: New test cases forclosemethod look good.The test cases ensure that the
closemethod is correctly called and handles invalid arguments appropriately.
denops/@denops-private/host_test.ts (2)
18-18: New methodcloseadded to the service object looks good.The
closemethod is correctly added to the service object and is unimplemented for testing purposes.
111-124: New test cases forclosemethod look good.The test cases ensure that the
closemethod is correctly called and handles invalid arguments appropriately.
denops/@denops-private/host/vim_test.ts (1)
17-25: New methodcloseadded to the service object looks good.The
closemethod is correctly added to the service object and is unimplemented for testing purposes.
denops/@denops-private/testutil/with.ts (10)
1-9: New import statements and constants look good.The
channel,tap,origLog,origError, andnoopare correctly imported and defined for use in the functions.
11-16: New type definition forFnlooks good.The
Fntype now includesstdoutandstderrstreams, which are necessary for the new functionality.
58-59: New command for setting columns looks good.The command ensures that the output does not wrap unexpectedly.
85-86: New command for setting columns looks good.The command ensures that the output does not wrap unexpectedly.
97-103: New listener setup looks good.The listener is correctly set up to accept connections on a free port.
107-113: New environment variables look good.The environment variables are correctly set up to include the address of the listener.
117-122: New stream handling looks good.The
stdoutandstderrstreams are correctly piped through theTextDecoderStreamandtapfunctions if verbose mode is enabled.Tools
GitHub Check: codecov/patch
[warning] 120-122: denops/@denops-private/testutil/with.ts#L120-L122
Added lines #L120 - L122 were not covered by tests
123-127: New channel setup forstdoutandstderrlooks good.The channels are correctly set up to handle the
stdoutandstderrstreams.
130-135: New function parameters look good.The function now includes
stdoutandstderrstreams in the parameters.
138-149: New process cleanup looks good.The process cleanup code correctly handles the
stdoutandstderrstreams.Tools
GitHub Check: codecov/patch
[warning] 141-142: denops/@denops-private/testutil/with.ts#L141-L142
Added lines #L141 - L142 were not covered by tests
denops/@denops-private/host/nvim_test.ts (1)
22-22: LGTM!The destructuring of
fnfunction parameters improves readability and the addition of theclosemethod aligns with the new functionality.
autoload/denops/_internal/server/chan.vim (6)
72-85: LGTM!The timeout and force close logic are appropriate and well-implemented.
114-116: LGTM!The initialization of
s:chanis appropriate and well-implemented.
121-131: LGTM!The force close logic and debug logging are appropriate and well-implemented.
133-138: LGTM!The logic to clear the force close delayer is appropriate and well-implemented.
140-154: LGTM!The close event handling and reconnection logic are appropriate and well-implemented.
31-31: LGTM!The introduction of the
l:ivariable for retry attempts is appropriate.Verification successful
LGTM!
The introduction of the
l:ivariable for retry attempts is appropriate. The functiondenops#_internal#server#chan#connectis used correctly in the following locations:
autoload/denops/server.vimautoload/denops/_internal/server/chan.vimScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the function `denops#_internal#server#chan#connect`. # Test: Search for the function usage. Expect: Correct usage of the function. rg --type vim $'denops#_internal#server#chan#connect'Length of output: 370
autoload/denops/server.vim (8)
22-36: LGTM!The status checks and logging are appropriate and well-implemented.
48-59: LGTM!The status checks and logging are appropriate and well-implemented.
67-67: LGTM!The asynchronous restart logic is appropriate and well-implemented.
67-69: LGTM!The force stop logic is appropriate and well-implemented.
77-94: LGTM!The status checks and logging are appropriate and well-implemented.
98-102: LGTM!The status checks and logging are appropriate and well-implemented.
110-110: LGTM!The asynchronous reconnection logic is appropriate and well-implemented.
122-128: LGTM!The new states
closingandclosedare appropriate and well-implemented.
doc/denops.txt (2)
157-161: LGTM!The new variable
g:denops#server#close_timeouthas been added with appropriate description.
307-308: LGTM!The new status
closingandclosedadded to the functiondenops#server#statusare appropriate and well-written.
denops/@denops-private/worker_test.ts (2)
Line range hint
209-237: Ensure proper synchronization and error handling withdoautocmdexecution.The
doautocmdcommand execution looks correct, but ensure that any asynchronous operations depending on this are properly synchronized and error-handled to avoid race conditions.Verification successful
Ensure proper synchronization and error handling with
doautocmdexecution.The functions using
doautocmdare invoked within an asynchronous context usingautocmdwith the++nestedflag, and proper error handling is implemented usingtry-finallyblocks.
autoload/denops/server.vimautoload/denops/_internal/plugin.vimautoload/denops/_internal/server/chan.vimThese findings confirm that proper synchronization and error handling are in place for
doautocmdexecution.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper synchronization and error handling with `doautocmd` execution. # Test: Search for `doautocmd` usage. Expect: Proper synchronization and error handling. rg --type vimscript $'doautocmd'Length of output: 1131
Script:
#!/bin/bash # Description: Extract surrounding code for each `doautocmd` usage to inspect synchronization and error handling mechanisms. # Extract 10 lines before and after each `doautocmd` usage for context. rg --type vimscript --context 10 'doautocmd'Length of output: 12442
Script:
#!/bin/bash # Description: Search for function calls where `doautocmd` is used to inspect synchronization. # Search for function definitions containing `doautocmd` to get function names. functions=$(rg --type vimscript --context 0 'doautocmd' | rg --type vimscript --context 10 'function!' | awk '/function!/ {print $2}') # Search for function calls for each function containing `doautocmd`. for func in $functions; do echo "Searching for calls to function: $func" rg --type vimscript "\b$func\b" doneLength of output: 137
Script:
#!/bin/bash # Description: Refine search for function calls where `doautocmd` is used to inspect synchronization. # Search for function definitions containing `doautocmd` to get function names. functions=$(rg --type vimscript --context 10 'doautocmd' | rg --type vimscript --context 10 'function!' | awk '/function!/ {print $2}') # Search for function calls for each function containing `doautocmd`. for func in $functions; do echo "Searching for calls to function: $func" rg --type vimscript "\b$func\b" doneLength of output: 4759
Line range hint
458-471: Ensure correct handling of SIGINT signal.The handling of the SIGINT signal should be verified to ensure that it properly disposes of resources and cleans up as expected.
tests/denops/server_test.ts (4)
119-310: Verify the correctness ofdenops#server#start()tests.The tests for
denops#server#start()should be verified to ensure they accurately cover all scenarios, including edge cases and error handling.Verification successful
Verification of
denops#server#start()tests:The tests for
denops#server#start()appear to be comprehensive, covering various scenarios, including edge cases and error handling.
- Instances found in
tests/denops/server_test.tsconfirm the presence and likely comprehensiveness of the tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#start()` tests. # Test: Search for `denops#server#start` tests. Expect: Comprehensive coverage of all scenarios, including edge cases and error handling. rg --type typescript $'denops#server#start'Length of output: 77
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#start()` tests. # Test: Search for `denops#server#start` tests. Expect: Comprehensive coverage of all scenarios, including edge cases and error handling. rg --type ts $'denops#server#start'Length of output: 1137
313-441: Verify the correctness ofdenops#server#stop()tests.The tests for
denops#server#stop()should be verified to ensure they accurately cover all scenarios, including edge cases and error handling.Verification successful
Verified the correctness of
denops#server#stop()tests.The tests for
denops#server#stop()intests/denops/server_test.tscomprehensively cover various scenarios, including edge cases and error handling.
tests/denops/server_test.ts: Multiple instances ofdenops#server#stopcalls.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#stop()` tests. # Test: Search for `denops#server#stop` tests. Expect: Comprehensive coverage of all scenarios, including edge cases and error handling. rg --type typescript $'denops#server#stop'Length of output: 76
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#stop()` tests. # Test: Search for `denops#server#stop` tests. Expect: Comprehensive coverage of all scenarios, including edge cases and error handling. rg 'denops#server#stop'Length of output: 1092
Line range hint
18-111: Verify the correctness ofdenops#server#status()tests.The tests for
denops#server#status()should be verified to ensure they accurately cover all possible states and transitions.Verification successful
Verified: The tests for
denops#server#status()cover all possible states and transitions comprehensively.
- The search results show multiple instances of
denops#server#statuswithin thetests/denops/server_test.tsfile, indicating thorough testing of various server states and transitions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#status()` tests. # Test: Search for `denops#server#status` tests. Expect: Comprehensive coverage of all states and transitions. rg --type typescript $'denops#server#status'Length of output: 78
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#status()` tests. # Test: Search for `denops#server#status` tests within TypeScript files. rg 'denops#server#status' --glob '*.ts'Length of output: 6388
443-937: Verify the correctness ofdenops#server#connect()anddenops#server#close()tests.The tests for
denops#server#connect()anddenops#server#close()should be verified to ensure they accurately cover all scenarios, including edge cases and error handling.Verification successful
Let's re-run the search without specifying the file type to ensure we capture all relevant instances of
denops#server#connectanddenops#server#close.
Tests for
denops#server#connect()anddenops#server#close()are present and cover various scenarios.The search results confirm that the tests for
denops#server#connect()anddenops#server#close()are present in thetests/denops/server_test.tsfile and cover various scenarios, including edge cases and error handling.
tests/denops/server_test.ts: Multiple references todenops#server#connecttests/denops/server_test.ts: Multiple references todenops#server#closeScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#connect()` and `denops#server#close()` tests. # Test: Search for `denops#server#connect` and `denops#server#close` tests. Expect: Comprehensive coverage of all scenarios, including edge cases and error handling. rg --type typescript $'denops#server#connect' rg --type typescript $'denops#server#close'Length of output: 157
Script:
#!/bin/bash # Description: Verify the correctness of `denops#server#connect()` and `denops#server#close()` tests. # Test: Search for `denops#server#connect` and `denops#server#close` tests. rg 'denops#server#connect' rg 'denops#server#close'Length of output: 2853
| import { | ||
| assert, | ||
| assertEquals, | ||
| assertFalse, | ||
| assertMatch, | ||
| assertRejects, | ||
| } from "jsr:@std/[email protected]"; | ||
| import { delay } from "jsr:@std/async@^0.224.0/delay"; | ||
| import { AsyncDisposableStack } from "jsr:@nick/[email protected]/async-disposable-stack"; | ||
| import { testHost } from "../../denops/@denops-private/testutil/host.ts"; | ||
| import { useSharedServer } from "../../denops/@denops-private/testutil/shared_server.ts"; | ||
| import { wait } from "../../denops/@denops-private/testutil/wait.ts"; | ||
|
|
||
| const MESSAGE_DELAY = 200; | ||
|
|
There was a problem hiding this comment.
Ensure consistency in import statements.
The import statements should be consistent in terms of formatting and order. Consider grouping similar imports together and separating them with a blank line for better readability.
import {
assert,
assertEquals,
assertFalse,
assertMatch,
assertRejects,
} from "jsr:@std/[email protected]";
import { delay } from "jsr:@std/async@^0.224.0/delay";
import { AsyncDisposableStack } from "jsr:@nick/[email protected]/async-disposable-stack";
import { testHost } from "../../denops/@denops-private/testutil/host.ts";
import { useSharedServer } from "../../denops/@denops-private/testutil/shared_server.ts";
import { wait } from "../../denops/@denops-private/testutil/wait.ts";
const MESSAGE_DELAY = 200;Committable suggestion was skipped due to low confidence.
autoload/denops/_internal/job.vim
Outdated
| \} | ||
| return job_start(a:args, l:options) | ||
| let l:job = job_start(a:args, l:options) | ||
| if l:job->job_status() !=# "run" |
There was a problem hiding this comment.
📝 I didn't realized but Neovim supports method syntax from Neovim 0.6.0
|
@Milly
Got it. Let me know when rebased @Milly |
- Add new `denops-variable`:
- `g:denops#server#close_timeout`
- Add new state for `denops#server#status()`:
- `closing`
- `closed`
- Change behavior `denops-function`:
- `denops#server#start()` can now be called even when status is
`closing`. In that case, the status will become `stopped` and then
restart asynchronously.
- `denops#server#stop()` is changed to asynchronously. It waits for
the server to close gracefully, and force terminate if timeouted.
- `denops#server#restart()` is changed to asynchronously. Perform the
stop and start steps above.
Milly
force-pushed
the
async-server-start-stop
branch
from
5e7e3c2 to
f766e94
Compare
|
I rebased it. @lambdalisue |
2 participants
denops-variable:g:denops#server#close_timeoutdenops#server#status():closingcloseddenops-function:denops#server#start()can now be called even when status isclosing. In that case, the status will becomestoppedand then restart asynchronously.denops#server#start()is changed to asynchronously.closing. In that case, the status will becomestoppedand then restart asynchronously.retry_threshold), it retries asynchronously (usesrestart_threshold).denops#server#stop()is changed to asynchronously.denops#server#restart()is changed to asynchronously.Summary by CodeRabbit
New Features
close()method to theServiceinterface for better resource management.stdoutandstderr.Bug Fixes
Refactor
forloops withwhileloops for better connection retries.Documentation
Tests