Refactor #18
Refactor #18
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
| @@ -1,32 +1,13 @@ | |||
| import { Denops } from "https://deno.land/x/denops_std@v5.0.1/mod.ts"; | |||
| import { Denops } from "https://deno.land/x/denops_std@v6.4.0/mod.ts"; | |||
There was a problem hiding this comment.
( ；´。 ｀；)なんかバージョンが違ってた
| @@ -1,6 +1,6 @@ | |||
| import { Denops } from "https://deno.land/x/denops_std@v6.4.0/mod.ts"; | |||
| import * as v from "https://deno.land/x/denops_std@v6.4.0/variable/mod.ts"; | |||
| import { emit } from "https://deno.land/x/denops_std@v6.5.1/autocmd/mod.ts"; | |||
| import { emit } from "https://deno.land/x/denops_std@v6.4.0/autocmd/mod.ts"; | |||
There was a problem hiding this comment.
( ；´。 ｀；)なんかバージョンが違ってた2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
denops/aider/main.ts (1)
147-163: Ensure correct relative path in .aiderignoreThe
forAiderIgnorePathis calculated by replacing thegitRootincurrentFilewith an empty string:const forAiderIgnorePath = currentFile.replace(gitRoot, "");This might result in a leading slash in the path, which could affect pattern matching in
.aiderignore. Consider adjusting the path to ensure it is relative without a leading slash.Apply this diff to adjust the path:
- const forAiderIgnorePath = currentFile.replace(gitRoot, ""); + const forAiderIgnorePath = currentFile.replace(`${gitRoot}/`, "");This change removes the leading slash, resulting in a relative path that matches the file system structure.
denops/aider/buffer.ts (1)
223-229: Update documentation to reflect the new return type ofidentifyAiderBufferThe
identifyAiderBufferfunction now returns aPromise<{ job_id: number; winnr: number; bufnr: number } | undefined>instead of using a callback. Consider updating the JSDoc@returnsannotation to accurately reflect this change.Apply this diff to update the documentation:
/** * 開いているウィンドウの中からAiderバッファを識別し、そのジョブID、ウィンドウ番号、バッファ番号を返します。 * - * @returns {Promise<{ job_id: number, winnr: number, bufnr: number }>} + * @returns {Promise<{ job_id: number; winnr: number; bufnr: number } | undefined>} */denops/aider/aiderCommand.ts (1)
39-43: Inconsistent documentation languageThe function documentation for
sendPromptmixes Japanese and English. For consistency and readability, consider using a single language for all comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- denops/aider/aiderCommand.ts (2 hunks)
- denops/aider/buffer.ts (9 hunks)
- denops/aider/main.ts (6 hunks)
- tests/aider_test.ts (1 hunks)
🔇 Additional comments (13)
tests/aider_test.ts (2)
Line range hint
1-8: Approve file renaming to align with testing framework.The renaming of the test file to
aider_test.tsis a positive change that ensures the file is correctly recognized by the testing framework. This change improves the project's adherence to testing conventions and should help in automating test discovery and execution.
5-5: Verify NeoVim-specific functionality and update test if necessary.The test case has been updated to focus on NeoVim instead of Vim. While this change aligns the test name and description with the project's apparent shift towards NeoVim, there are a few points to consider:
Ensure that this change is intentional and aligns with the project's goals. If the project is meant to support both Vim and NeoVim, consider keeping tests for both or clarifying the scope in the project documentation.
The test functionality (
getCurrentFilePath) remains unchanged. Verify if this function behaves identically in NeoVim and Vim environments. If there are NeoVim-specific features or behaviors that should be tested, consider updating the test accordingly.If this change represents a broader shift in the project's focus, ensure that other parts of the codebase and documentation reflect this change to maintain consistency.
To ensure the change is consistent across the project, run the following script:
This script will help identify any inconsistencies in the project's references to Vim or NeoVim, allowing you to update them accordingly.
denops/aider/main.ts (8)
2-2: Importing function utilities from denops_stdThe import of the
fnmodule fromdenops_stdprovides essential Vim function utilities needed throughout the script.
5-5: Importing utility functions from utils.tsImporting
getAiderBufferNrandgetCurrentFilePathfrom./utils.tsis appropriate and allows reuse of utility functions within the main script.
67-67: Updated sendPrompt command implementationThe
sendPromptcommand now usesbuffer.sendPromptByBuffer, which aligns with the new implementation that handles input as a string. This change ensures the prompt is sent correctly based on the open buffer type.
81-81: Utilizing buffer.sendPromptWithInput in addFile commandThe
addFilecommand correctly usesbuffer.sendPromptWithInputto send the/addprompt with the specified file path. This change simplifies the command handling by directly sending the prompt with user input.
88-99: Direct implementation of addCurrentFile commandThe
addCurrentFilecommand now includes its implementation directly within thecommandfunction. The logic correctly:
- Retrieves the current buffer number.
- Checks if the Aider buffer exists; if not, it runs
aiderCommand.silentRun(denops).- Verifies that the current buffer is not a terminal buffer to prevent unintended behavior.
- Retrieves the current file path.
- Sends the
/addprompt with the current file.This refactoring enhances code clarity by localizing command-specific logic.
107-107: Using buffer.sendPromptWithInput in addWeb commandThe
addWebcommand appropriately usesbuffer.sendPromptWithInputto send the/webprompt with the provided URL. This aligns with the updated approach of handling commands directly.
116-116: Using buffer.sendPromptWithInput in ask commandThe
askcommand correctly utilizesbuffer.sendPromptWithInputto send the/askprompt with the user's question. This change streamlines the command execution process.
175-175: Using buffer.sendPromptWithInput in test commandThe
testcommand properly usesbuffer.sendPromptWithInputto send the/testprompt with the specified command. This aligns with the standardization of command implementations across the codebase.denops/aider/buffer.ts (1)
88-88: Verify all calls tosendPromptWithInputinclude the newinputparameterThe
sendPromptWithInputfunction signature now includes aninputparameter. Ensure that all calls to this function throughout the codebase have been updated to include this parameter to prevent potential runtime errors.Run the following script to identify any outdated calls:
denops/aider/aiderCommand.ts (2)
1-4: LGTMThe updated import statements are correct and use the latest versions.
56-58: LGTMThe
exitfunction correctly sends the exit command and deletes the associated buffer.
| await command("openIgnore", "0", async () => { | ||
| const gitRoot = (await fn.system(denops, "git rev-parse --show-toplevel")) | ||
| .trim(); | ||
| const filePathToOpen = `${gitRoot}/.aiderignore`; | ||
| if (await fn.filereadable(denops, filePathToOpen)) { | ||
| await denops.cmd(`edit ${filePathToOpen}`); | ||
| return; | ||
| } | ||
| console.log(".aiderignoreファイルが見つかりません。"); | ||
| }), |
There was a problem hiding this comment.
Inconsistent language in log message
The log message on line 142 is in Japanese:
console.log(".aiderignoreファイルが見つかりません。");For consistency and to accommodate all users, consider translating this message to English to match the rest of the codebase.
Apply this diff to update the log message:
- console.log(".aiderignoreファイルが見つかりません。");
+ console.log(".aiderignore file not found.");| async () => { | ||
| { | ||
| const currentFile = await getCurrentFilePath(denops); | ||
|
|
||
| const gitRoot = | ||
| (await fn.system(denops, "git rev-parse --show-toplevel")) | ||
| .trim(); | ||
| const filePathToOpen = `${gitRoot}/.aiderignore`; | ||
| const forAiderIgnorePath = currentFile.replace(gitRoot, ""); | ||
|
|
||
| const file = await fn.readfile(denops, filePathToOpen); | ||
| file.push(`!${forAiderIgnorePath}`); | ||
|
|
||
| await fn.writefile(denops, file, filePathToOpen); | ||
| console.log(`Added ${currentFile} to .aiderignore`); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add error handling for missing .aiderignore file
In the addIgnoreCurrentFile command, there is no check to verify if the .aiderignore file exists before attempting to read it with fn.readfile. If the file does not exist, this could result in an error. Consider adding a check to ensure the file exists, and create it if necessary.
Apply this diff to add error handling:
+ if (!(await fn.filereadable(denops, filePathToOpen))) {
+ await fn.writefile(denops, [], filePathToOpen);
+ }
const file = await fn.readfile(denops, filePathToOpen);This ensures that the .aiderignore file exists before reading and appending to it.
📝 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.
| async () => { | |
| { | |
| const currentFile = await getCurrentFilePath(denops); | |
| const gitRoot = | |
| (await fn.system(denops, "git rev-parse --show-toplevel")) | |
| .trim(); | |
| const filePathToOpen = `${gitRoot}/.aiderignore`; | |
| const forAiderIgnorePath = currentFile.replace(gitRoot, ""); | |
| const file = await fn.readfile(denops, filePathToOpen); | |
| file.push(`!${forAiderIgnorePath}`); | |
| await fn.writefile(denops, file, filePathToOpen); | |
| console.log(`Added ${currentFile} to .aiderignore`); | |
| } | |
| }, | |
| async () => { | |
| { | |
| const currentFile = await getCurrentFilePath(denops); | |
| const gitRoot = | |
| (await fn.system(denops, "git rev-parse --show-toplevel")) | |
| .trim(); | |
| const filePathToOpen = `${gitRoot}/.aiderignore`; | |
| const forAiderIgnorePath = currentFile.replace(gitRoot, ""); | |
| if (!(await fn.filereadable(denops, filePathToOpen))) { | |
| await fn.writefile(denops, [], filePathToOpen); | |
| } | |
| const file = await fn.readfile(denops, filePathToOpen); | |
| file.push(`!${forAiderIgnorePath}`); | |
| await fn.writefile(denops, file, filePathToOpen); | |
| console.log(`Added ${currentFile} to .aiderignore`); | |
| } | |
| }, |
| async function sendPromptFromSplitWindow( | ||
| denops: Denops, | ||
| prompt: string, | ||
| ): Promise<void> { | ||
| const aiderBuffer = await identifyAiderBuffer(denops); | ||
| if (aiderBuffer === undefined) { | ||
| return; | ||
| } | ||
| const { job_id, winnr } = aiderBuffer; | ||
|
|
||
| for (let winnr = 1; winnr <= totalWindows; winnr++) { | ||
| const bufnr = await denops.call("winbufnr", winnr); | ||
| if (await v.g.get(denops, "aider_buffer_open_type") !== "floating") { | ||
| await denops.cmd(`${winnr}wincmd w`); | ||
| } else { | ||
| const totalWindows = ensure<number>( | ||
| await denops.call("winnr", "$"), | ||
| is.Number, | ||
| ); | ||
|
|
||
| const buftype = await denops.call("getbufvar", bufnr, "&buftype"); | ||
| for (let winnr = 1; winnr <= totalWindows; winnr++) { | ||
| const bufnr = await denops.call("winbufnr", winnr); | ||
|
|
||
| if (buftype === "terminal") { | ||
| await denops.cmd(`${winnr}wincmd w`); | ||
| break; | ||
| } | ||
| const buftype = await denops.call("getbufvar", bufnr, "&buftype"); | ||
|
|
||
| if (buftype === "terminal") { | ||
| await denops.cmd(`${winnr}wincmd w`); | ||
| break; | ||
| } | ||
| } | ||
| await feedkeys(denops, "G"); | ||
| await feedkeys(denops, '"qp'); | ||
| await denops.call("chansend", job_id, "\n"); | ||
| await denops.cmd("wincmd p"); | ||
| }); | ||
| } | ||
| await aiderCommand.sendPrompt(denops, job_id, prompt); |
There was a problem hiding this comment.
Review logic for handling buffer types in sendPromptFromSplitWindow
In the sendPromptFromSplitWindow function, when aider_buffer_open_type is "floating", the code iterates through all windows to find a terminal buffer. This may be counterintuitive since a floating buffer type typically wouldn't require switching windows. Consider reviewing the condition to ensure the logic aligns with the intended behavior for different buffer types.
| const buffer = await identifyAiderBuffer(denops); | ||
| if (buffer === undefined) { | ||
| return; | ||
| } | ||
| const { job_id, bufnr } = buffer; | ||
| aiderCommand.exit(denops, job_id, bufnr); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add user feedback when no Aider buffer is found
In the exitAiderBuffer function, if identifyAiderBuffer returns undefined, the function silently returns without notifying the user. Consider informing the user that no Aider buffer is currently open to improve usability and debugging.
Apply this diff to provide user feedback:
if (buffer === undefined) {
+ await denops.cmd("echo 'No Aider buffer is currently open.'");
return;
}📝 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.
| const buffer = await identifyAiderBuffer(denops); | |
| if (buffer === undefined) { | |
| return; | |
| } | |
| const { job_id, bufnr } = buffer; | |
| aiderCommand.exit(denops, job_id, bufnr); | |
| const buffer = await identifyAiderBuffer(denops); | |
| if (buffer === undefined) { | |
| await denops.cmd("echo 'No Aider buffer is currently open.'"); | |
| return; | |
| } | |
| const { job_id, bufnr } = buffer; | |
| aiderCommand.exit(denops, job_id, bufnr); |
| async sendPromptByBuffer( | ||
| denops: Denops, | ||
| openBufferType: BufferLayout, | ||
| ): Promise<void> { | ||
| // テキストを取得してプロンプト入力ウインドウを閉じる | ||
| await feedkeys(denops, 'ggVG"qy'); | ||
| const bufferContent = ensure( | ||
| await denops.call("getbufline", "%", 1, "$"), | ||
| is.ArrayOf(is.String), | ||
| ).join("\n"); | ||
|
|
||
| await denops.cmd("bdelete!"); | ||
|
|
||
| openBufferType === "floating" | ||
| ? await sendPromptFromFloatingWindow(denops) | ||
| : await sendPromptFromSplitWindow(denops); | ||
| if (openBufferType === "floating") { | ||
| await sendPromptFromFloatingWindow(denops, bufferContent); | ||
| } else { | ||
| await sendPromptFromSplitWindow(denops, bufferContent); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Outdated References to sendPrompt Found
The following references to sendPrompt still exist and need to be updated to sendPromptByBuffer:
denops/aider/aiderCommand.ts:async sendPrompt(denops/aider/buffer.ts:await aiderCommand.sendPrompt(denops, jobId, prompt);await aiderCommand.sendPrompt(denops, job_id, prompt);
🔗 Analysis chain
Ensure all references to sendPrompt are updated to sendPromptByBuffer
The method sendPrompt has been renamed to sendPromptByBuffer. Please verify that all references to the old method name have been updated to prevent undefined method errors.
Use this script to find any outdated references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any references to the old method name `sendPrompt`
# Expected Result: No occurrences of `sendPrompt` except in `sendPromptByBuffer`.
rg --type ts 'sendPrompt\('
Length of output: 237
| }, | ||
| async sendPrompt( | ||
| async sendPromptByBuffer( |
There was a problem hiding this comment.
実装内容に合わせて微妙にリネーム
| const prompt = `/add ${currentFile}`; | ||
| async sendPrompt( | ||
| denops: Denops, | ||
| jobId: number, |
There was a problem hiding this comment.
テストの実装方針によってはaiderBufferのjobIdを取るところまでaiderCommandモジュールの責務にした方がいいかもしれないがどっちが正しいかは微妙なところ
| @@ -316,31 +314,34 @@ async function sendPromptFromFloatingWindow(denops: Denops): Promise<void> { | |||
| * | |||
| * @param {Denops} denops - Denopsインスタンス | |||
| */ | |||
| async function sendPromptFromSplitWindow(denops: Denops): Promise<void> { | |||
| await identifyAiderBuffer(denops, async (job_id, winnr, _bufnr) => { | |||
There was a problem hiding this comment.
このへんから見た目複雑に差分が出てしまっているけれどidentifyAiderBufferとsendPromptのインターフェースが変わった部分以外は変えてないはず
| @@ -1,32 +1,13 @@ | |||
| import { Denops } from "https://deno.land/x/denops_std@v5.0.1/mod.ts"; | |||
| import { Denops } from "https://deno.land/x/denops_std@v6.4.0/mod.ts"; | |||
|
|
||
| await emit(denops, "User", "AiderOpen"); | ||
| return; | ||
| }, | ||
|
|
||
| async sendPromptWithInput(denops: Denops): Promise<void> { | ||
| async sendPromptWithInput(denops: Denops, input: string): Promise<void> { |
There was a problem hiding this comment.
テストしやすさを考えると、確かにinputを引数にするほうがよさそう（当時はテストを1ミリも考えていなかった顔
| denops: Denops, | ||
| openBufferType: BufferLayout, | ||
| ): Promise<void> { | ||
| // テキストを取得してプロンプト入力ウインドウを閉じる | ||
| await feedkeys(denops, 'ggVG"qy'); | ||
| const bufferContent = ensure( |
There was a problem hiding this comment.
普通にこれでいいですね…（当時は覚えたてのfeedkeysを使いまくっていた記憶
There was a problem hiding this comment.
https://zenn.dev/uga_rosa/articles/200ad8013db7a8 feedkeysは非同期処理らしいので現状の使用箇所は処理落ち時とかにバグってる可能性ありそう ( ；´。 ｀；)
| @@ -215,19 +220,13 @@ export const buffer = { | |||
| }; | |||
|
|
|||
| /** | |||
| * 開いているウィンドウの中からターミナルバッファを識別し、そのジョブID、ウィンドウ番号、バッファ番号をコールバック関数に渡します。 | |||
| * 開いているウィンドウの中からAiderバッファを識別し、そのジョブID、ウィンドウ番号、バッファ番号を返します。 | |||
There was a problem hiding this comment.
コールバック、async/awaitとかの非同期処理をいい感じに書く機能がない言語で非同期処理やる場合に仕方なく使うものだとおもてる
あとはサーバーとかを挟んでいてその手の機能がうまく使えない場合
#17
Summary by CodeRabbit
New Features
Bug Fixes
.aiderignore, streamlining user experience.Documentation
Refactor