-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
show buffers on assertion failure #29
show buffers on assertion failure #29
Conversation
WalkthroughThe changes involve modifications to the assertion functions in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
CIテストまわっとるな ヨシ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/assertions.ts (2)
22-26
: Enhanced error reporting for hidden buffer assertionThe changes improve error reporting and readability. For consistency with other assertions, consider using template literals for multi-line strings.
Consider refactoring the assertion message to use template literals for consistency:
assert( buf.winnr === undefined, `Aider buffer is shown in a window when it should be hidden. ${await bufferStateMessage(denops)}`, );
51-60
: New helper function for buffer state reportingThe
bufferStateMessage
function effectively supports the PR objective by providing detailed buffer information. It's well-structured and improves debugging capabilities.Consider the following optimizations:
- For better performance with large numbers of buffers, consider using a more efficient string concatenation method, such as an array of strings joined at the end.
- The function is marked as async, but there are no await statements within the loop. Consider using
Promise.all
to parallelize buffer information retrieval for potential performance gains.Here's a potential optimization:
async function bufferStateMessage(denops: Denops): Promise<string> { const bufCount = ensure(await denops.call("bufnr", "$"), is.Number); const bufferPromises = Array.from({ length: bufCount }, async (_, i) => { const bufnr = i + 1; const [bufname, bufwinnr] = await Promise.all([ denops.call("bufname", bufnr), denops.call("bufwinnr", bufnr), ]); return JSON.stringify({ bufnr, bufname: ensure(bufname, is.String), bufwinnr: ensure(bufwinnr, is.Number), }); }); const bufferStates = await Promise.all(bufferPromises); return bufferStates.join("\n") + "\n"; }This version parallelizes the buffer information retrieval and uses an array join for string concatenation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/assertions.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/assertions.ts (4)
13-14
: Improved error messages for better debuggingThe changes in these lines enhance the error reporting by providing more context when assertions fail. The use of
bufferStateMessage
aligns well with the PR objective of showing buffer information on assertion failures.
34-34
: Improved error message for buffer alive assertionThis change enhances the error reporting by providing more context when the assertion fails, consistent with the improvements in other functions.
48-48
: Enhanced error message for buffer content mismatchThis change improves error reporting by clearly showing the difference between expected and actual buffer content. While not directly related to showing buffer states on assertion failure, it enhances overall debugging capabilities.
Line range hint
1-60
: Overall assessment: Well-implemented feature with room for minor improvementsThe changes successfully implement the feature of showing buffer information on assertion failures, aligning well with the PR objectives. The error messages have been significantly improved across all assertion functions, enhancing debugging capabilities.
Key points:
- Error messages now provide detailed buffer state information.
- The new
bufferStateMessage
function effectively supports the main objective.- Code consistency and readability have been maintained.
Consider implementing the suggested optimizations for the
bufferStateMessage
function and ensuring consistency in string formatting across assertions. These minor improvements will further enhance the overall quality of the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
Summary by CodeRabbit
Bug Fixes
New Features