Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe shared Tree-sitter parser callback type now accepts an optional line number. ChangesTree-sitter line parser
Estimated code review effort: 2 (Simple) | ~8 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/fzf-lua/win/tsinjector.lua (1)
130-131:⚠️ Potential issue | 🟡 MinorUpdate parameter type annotation for
line_parser.The
line_parserparameter type annotation at line 130 also needs updating to include thelnumparameter. This will resolve the remaining[redundant-parameter]warning at line 174.🔧 Proposed fix
----@param line_parser (fun(line: string):string?,string?,string?,string?)|boolean? +---@param line_parser (fun(line: string, lnum: integer):string?,string?,string?,string?)|boolean? ---@return function detach🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win/tsinjector.lua` around lines 130 - 131, Update the type annotation for the parameter named line_parser in tsinjector.lua to include the lnum argument (e.g., change (fun(line: string):string?,string?,string?,string?)|boolean? to a signature that accepts (line: string, lnum: number) and returns the four optional strings or nil); locate the annotation above the function that declares/detects line_parser and add the lnum parameter to match the runtime usage (this will remove the [redundant-parameter] warning at the call site around line 174).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/win/tsinjector.lua`:
- Around line 135-136: The type annotation for default_line_parser is out of
sync with its implementation and types.lua: update the local annotation for
default_line_parser to include the second parameter (lnum) so the signature
matches the actual function (e.g., fun(line: string, lnum: number):
string?,string?,string?,string?) and remove the redundant-parameter warning;
target the annotation immediately above the default_line_parser function to
correct the mismatch.
- Line 174: Update the _treesitter parser functions to accept the second ignored
parameter by changing their signatures from function(line) to function(line, _);
specifically modify the _treesitter implementations named git_log,
command_history, and search_history (the functions currently declared with a
single parameter) so they match the default tsinjector.lua signature
function(line, _) and thereby explicitly accept the lnum/second argument passed
by line_parser(line:sub(min_col), i).
---
Outside diff comments:
In `@lua/fzf-lua/win/tsinjector.lua`:
- Around line 130-131: Update the type annotation for the parameter named
line_parser in tsinjector.lua to include the lnum argument (e.g., change
(fun(line: string):string?,string?,string?,string?)|boolean? to a signature that
accepts (line: string, lnum: number) and returns the four optional strings or
nil); locate the annotation above the function that declares/detects line_parser
and add the lnum parameter to match the runtime usage (this will remove the
[redundant-parameter] warning at the call site around line 174).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36a25265-8532-4dd9-bb2f-538977ff4db4
📒 Files selected for processing (2)
lua/fzf-lua/types.lualua/fzf-lua/win/tsinjector.lua
|
-> Fix 2633-1 -> #2632 This is a hack but not sure if there better idea |
|
Just following up on this since I'm interested in fixing #2632. Are the lint failures the only blockers preventing this from being merged? |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
I just wonder is there any better way to do it. But should be no problem to change it. This is still a internal opts with few users. I'll resolve the lint when available. |
It's non trivial to determine where the "prompt" line is.
When we have reverse/default layout, info/header/footer border
This is a workaround for user find the prompt hl annoying:
opts.command_history._treesitter = function(line, lnum)
if lnum == 1 then return end
return line:match("(.-):?(%d+)[: ](.+)$")
end
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
When query is at the first line: _treesitter = function(line, lnum)
if lnum == 1 then return end
return 'foo.vim', nil, line
end,cc @ibhagwan for review, not sure why but I could not merge it without review |
I disabled direct push to master enabled only via PRs - not sure why it wants my review, I’ll take a look at the ruleset. |
|
Changed it to required reviewers 0 - can you see if you can merge again? |
|
I approved this, not sure why it still needed my review after setting required reviewers to 0, maybe in a new PR or resubmitted commit it will work (since I was already required to review). |
|
I think I fixed it now, no reviews should be required |
3 participants
It's non trivial to determine where the "prompt" line is.
When we have reverse/default layout, info/header/footer border
This is a workaround for user find the prompt hl annoying:
Summary by CodeRabbit