-
Notifications
You must be signed in to change notification settings - Fork 209
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
Edit: Support trimming active selections to a tree-sitter node #4031
Edit: Support trimming active selections to a tree-sitter node #4031
Conversation
Nice! |
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.
Great work! Left some comments inline.
P.S. It would be so cool to somehow animate the selection expansion to the nearest node (showing to a user how smart Cody is!). I really wanted to see visual feedback for it in the demo video. cc @toolmantim
vscode/src/commands/execute/doc.ts
Outdated
// No usable selection, fallback to expanding the range to the nearest block. | ||
return {} |
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.
Is the expansion to the nearest block done somewhere up the call chain?
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.
Yeah it's the default behavior for Edit
commands. It's a little confusing when presented like this but it makes sense as an abstraction (command specific ranges falling back to generic edit block ranges if not found).
Added a comment
// It is probable that the user would benefit from expanding to this node completely | ||
const selectionMatchesNode = | ||
adjustedSelection.start.isEqual(range.start) && range.contains(adjustedSelection.end) | ||
if (!selectionMatchesNode && !editor.selection.isEmpty) { |
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.
Should we cover this branching in one of the unit tests?
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.
Kinda tricky to isolate this with a specific test, but actually we have agent tests that cover this.
I noticed an issue with cursor position ranges (isEmpty
) from these tests already
Description
Fixes CODY-1363
This PR:
Before:
We would prioritise the users selection (if non-empty) before anything else. This worked in most cases, but was not optimal as the user may have actually been attempting to match the tree-sitter node, but made a mistake and possibly:
This would also be non-optimal as in some case we get more insight into the users' selection if we use tree-sitter. For example, for "Generate Documentation" in Python, using tree-sitter will help us determine the correct
insertionPoint
for the documentation. This relies on knowing the syntax of the node in Python. Even if the users' selection exactly matched the node we would not get that information.AdjustedRangeBefore.mov
After:
We now attempt to match the users' selection to a tree-sitter node, in a safe manner that doesn't discard situations where their selection is likely preferred. This is by:
AdjustedRangeAfter.mov
Test plan
Document:
console.log()
). Expect it to only document this line with the function callfunction foo() {\n console.log('hi')
func). Expect it to document the tree-sitter node correctly.