Fix denops#cache#update() #461
Conversation
Use loop
WalkthroughRefactors denops#cache#update to iterate over each entry file and invoke deno cache per file with optional --reload, starting and awaiting a Denops job per iteration. Removes batched invocation and final combined wait; retains final success/update messaging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Vim as Vim (denops#cache#update)
participant Denops as Denops
participant Deno as deno cache
User->>Vim: denops#cache#update({reload?})
Note over Vim: Build list: [s:mod] + plugin entry files
loop For each entry file
Vim->>Vim: Construct args (add --reload if requested)
Vim->>Denops: start job (deno cache <entryfile>)
Denops->>Deno: deno cache <entryfile>
Deno-->>Denops: exit status
Denops-->>Vim: job done
end
Vim-->>User: Echo final update message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 96.75% 96.75%
=======================================
Files 11 11
Lines 924 924
Branches 143 143
=======================================
Hits 894 894
Misses 27 27
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Plan: Pro
📒 Files selected for processing (1)
autoload/denops/cache.vim(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (macos-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (ubuntu-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (macos-latest, 2.3.0, v9.1.1646, v0.11.3)
- GitHub Check: test (windows-latest, 2.x, v9.1.1646, v0.11.3)
- GitHub Check: test (windows-latest, 2.3.0, v9.1.1646, v0.11.3)
🔇 Additional comments (5)
autoload/denops/cache.vim (5)
9-11: Message on non-reload path — LGTM.Helpful guidance for forcing reload. No change needed.
13-15: Per‑file caching loop — LGTM.Sequentializing isolates failures and matches the PR goal.
16-21: Argument construction — LGTM.Simple and clear; per‑file --reload wiring is correct.
32-32: No-op change.Nothing to flag here.
22-30: Handle per-file failures, respect each plugin’s cwd, and use string env values.Failures/timeouts currently report success and per-plugin deno.json import-maps can be skipped because cwd isn't set; env values should be strings for job portability.
- Initialize: let l:failures = [] before the loop.
- In the loop when starting the job: add cwd: fnamemodify(l:entryfile, ':h') and make env values strings (e.g. '1').
- Capture wait result: let l:done = denops#_internal#wait#for(60 * 1000, { -> s:job is# v:null }, 100).
- If !l:done || s:last_status != 0 then call add(l:failures, l:entryfile).
- At top-level: let s:last_status = 0 and update s:on_exit(job, status, event) to set s:last_status = a:status and let s:job = v:null.
This prevents false “success” and ensures each cache runs from the plugin directory so per‑plugin import‑maps are respected.
| \}) | ||
| call denops#_internal#wait#for(60 * 1000, { -> s:job is# v:null }, 100) | ||
|
|
||
| echomsg '[denops] Deno cache is updated.' |
There was a problem hiding this comment.
Report failures instead of unconditional success.
End with an error summary when any file fails.
- echomsg '[denops] Deno cache is updated.'
+ if empty(l:failures)
+ echomsg '[denops] Deno cache is updated.'
+ else
+ echohl WarningMsg
+ echomsg printf('[denops] Deno cache finished with errors: %d failure(s).', len(l:failures))
+ for l:f in l:failures
+ echomsg printf('[denops] failed: %s', l:f)
+ endfor
+ echohl None
+ endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echomsg '[denops] Deno cache is updated.' | |
| if empty(l:failures) | |
| echomsg '[denops] Deno cache is updated.' | |
| else | |
| echohl WarningMsg | |
| echomsg printf('[denops] Deno cache finished with errors: %d failure(s).', len(l:failures)) | |
| for l:f in l:failures | |
| echomsg printf('[denops] failed: %s', l:f) | |
| endfor | |
| echohl None | |
| endif |
🤖 Prompt for AI Agents
In autoload/denops/cache.vim around line 33, the code unconditionally echoes a
success message; change it to collect failures during the caching loop (e.g.,
keep a list or counter of failed files when file operations return error), and
after the loop check that collection: if there are failures echoerr a concise
error summary including the failure count (and optionally the filenames) and
return a non-zero/false result, otherwise echomsg the success message; ensure
any error paths push the failed file info into the collection so the final
summary is accurate.
Use loop to fix import error like this.
Summary by CodeRabbit