👍 Add getbufinfo() helper function #279
Conversation
|
Warning Rate limit exceeded@mityu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request introduces a new Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 85.73% 85.80% +0.06%
==========================================
Files 64 65 +1
Lines 3442 3458 +16
Branches 301 301
==========================================
+ Hits 2951 2967 +16
Misses 489 489
Partials 2 2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
function/buffer.ts (1)
417-419: Review the in-code docs referencing the builtin function names.Here, the documentation states:
“CAUTION: Different from the builtin getbufnr() function...”
However, the surrounding context is about
getbufinfo(). It's potentially confusing for future maintainers to seegetbufnr()referenced here. Confirm whether you intended to reference the builtingetbufinfo()function instead ofgetbufnr(), then adjust the documentation accordingly for clarity.-CAUTION: Different from the builtin getbufnr() function... +CAUTION: Different from the builtin getbufinfo() function...Also applies to: 443-446
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
function/buffer.ts(3 hunks)function/buffer_test.ts(2 hunks)
🔇 Additional comments (3)
function/buffer_test.ts (2)
64-65: Good test coverage addition.
This test sequence (await denops.cmd("1,$bwipeout!");) ensures that previously created buffers are removed before verifying the new filtering behavior. No issues found here.
66-75: Test logic aligns well with the new filtering requirement.
The test successfully verifies that the getbufinfo() function excludes the "variables" key by asserting the key's absence in the returned object. This addresses the PR’s objective of preventing serialization problems caused by buffer-local variables. Good use of assertFalse.
function/buffer.ts (1)
460-465: Filtering out the variables property is implemented correctly.
The usage of denops.eval along with the map and filter logic accurately omits variables from the returned buffer info. This resolves the issue of funcrefs in buffer-local variables causing JSON serialization errors. No further changes needed.
|
Hmm..., test fails due to the changes to the doc... |
|
@mityu Could you move the implementation into |
|
I'm sleepy today. I'll do remaining work tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
helper/getbufinfo.ts (2)
5-7: Add JSDoc comments for clarity.The
TODO: writeplaceholder can be updated with a concise description explaining what thegetbufinfofunction does, its parameters, and return types. This makes the code more maintainable and user-friendly./** - * TODO: write + * Retrieve information about the specified buffer(s) in a Denops environment. + * If a buffer is specified by name or index, returns information for that buffer. + * If called with no arguments or dictionary arguments, returns information for all buffers. */
17-36: Validate potential null or undefined arguments.Although
argsis typedunknown[], in practice it should align with the overload signatures. You may consider an early check to avoid unexpected runtime errors if the call pattern changes.export async function getbufinfo( denops: Denops, ...args: unknown[] ): Promise<BufInfo[]> { + // Example safeguard if usage might vary in the future + if (!args) { + return []; + } const bufinfos = await denops.eval( "map(call('getbufinfo', l:args), {_, v -> filter(v, {k -> k !=# 'variables'})})", {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
helper/getbufinfo.ts(1 hunks)helper/getbufinfo_test.ts(1 hunks)helper/mod.ts(1 hunks)
🔇 Additional comments (6)
helper/mod.ts (1)
12-12: Exporting new module is good to have!
This export line cleanly exposes the newly created getbufinfo.ts module to external consumers. This is consistent with the approach used for other helper modules and doesn't appear to break any existing interfaces.
helper/getbufinfo.ts (2)
1-3: Imports are correct and cohesive.
All imported types correlate well with the usage in the file. This ensures type safety for Denops, BufInfo, and the dictionary arguments.
8-16: Method overloading is consistent with typical usage patterns.
The overload definitions for different argument types are a great approach to provide flexible call signatures. However, ensure to maintain consistent naming or comments for each overload signature, so developers understand how they differ.
helper/getbufinfo_test.ts (3)
1-10: Solid test structure and usage of @denops/test.
The test scaffolding is well set up to confirm getbufinfo behavior in both normal and multi-buffer contexts.
11-60: Good coverage of various buffer states.
By creating multiple buffers, modifying lines, and toggling visibility and loading states, you’re effectively testing a variety of scenarios. This ensures getbufinfo reliably returns the correct boolean flags (changed, hidden, listed, loaded).
65-75: Buffer-local variable filtering works well.
The test confirms that variables key is removed as intended. The assertion assertFalse("variables" in actual[0]) is an effective check, preventing future regressions if the logic changes.
mityu
force-pushed
the
fix-getbufinfo
branch
from
0100ab6 to
57ead86
Compare
mityu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
helper/getbufinfo.ts (3)
5-23: Consider enhancing the example code.While the documentation is clear and well-written, the example could be more comprehensive by:
- Showing both overload usages (with
bufanddictarguments)- Including error handling
Here's a suggested enhancement:
import type { Entrypoint } from "jsr:@denops/std"; import { getbufinfo } from "jsr:@denops/std/helper/getbufinfo"; export const main: Entrypoint = async (denops) => { try { // Using buffer number const currentBufInfo = await getbufinfo(denops, await denops.call("bufnr") as number); console.log("Current buffer info:", currentBufInfo); // Using dict argument const allBufInfo = await getbufinfo(denops, { buflisted: 1 }); console.log("All listed buffers:", allBufInfo); } catch (error) { console.error("Failed to get buffer info:", error); } };
32-44: Consider using a more explicit type assertion.The type assertion could be simplified by directly asserting the expected type:
- ) as Record< - keyof BufInfo, - unknown - >[]; + ) as Partial<BufInfo>[];
45-52: Consider using explicit boolean conversion.While double negation (
!!) is a common pattern, consider usingBoolean()for more explicit type conversion:return bufinfos.map((bufinfo) => ({ ...bufinfo, - changed: !!bufinfo.changed, - hidden: !!bufinfo.hidden, - listed: !!bufinfo.listed, - loaded: !!bufinfo.loaded, + changed: Boolean(bufinfo.changed), + hidden: Boolean(bufinfo.hidden), + listed: Boolean(bufinfo.listed), + loaded: Boolean(bufinfo.loaded), } as unknown as BufInfo));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
helper/getbufinfo.ts(1 hunks)helper/getbufinfo_test.ts(1 hunks)helper/mod.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- helper/mod.ts
- helper/getbufinfo_test.ts
🧰 Additional context used
🪛 GitHub Actions: Test
helper/getbufinfo.ts
[error] 2-2: Unknown export './helper/getbufinfo' for '@denops/std'.
🔇 Additional comments (2)
helper/getbufinfo.ts (2)
1-3: LGTM! Imports are well-organized.The type imports are properly structured and include all necessary dependencies.
🧰 Tools
🪛 GitHub Actions: Test
[error] 2-2: Unknown export './helper/getbufinfo' for '@denops/std'.
24-31: LGTM! Function overloads are well-defined.The overload signatures clearly define the supported argument types and return type.
mityu
force-pushed
the
fix-getbufinfo
branch
2 times, most recently
from
5aa568f to
dd66c7b
Compare
|
I'm really sorry for the super late. I've updated the patch. Could you review again? @lambdalisue |
When a funcref appears in buffer-local variables, calling getbufinfo() from denops world will fail due to the error on serializing its return value using json_encode(). To avoid this, we add a wrapper function of getbufinfo() to filter-out buffer-local variables from its return value.
mityu
force-pushed
the
fix-getbufinfo
branch
from
dd66c7b to
c105415
Compare
|
(Dropped the first commit and the next reverting commit.) |
getbufnr()function have dictionaries of buffer local variables in its return value. Therefore, when there's a funcref in buffer local variables, denops will fail to send the return value back to the deno world.This behavior is inconvenient, this PR makes denops-std's
getbufnr()omit the "variables" entries from the return value.Reproduce Steps
/tmp/repro/denops/repro/main.tsvimrc.vimvim -u vimrc.vim:ReproEnvironment
Summary by CodeRabbit
New Features
Bug Fixes
Tests