-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Cody completions: Improve naming #52303
Conversation
abeatrix
left a comment
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.
Clean! I should do the same for inline assist too 😓
4b56496 to
200559c
Compare
valerybugakov
left a comment
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.
This change makes things much easier to understand for a newcomer. Thanks, @philipp-spiess!
client/cody/package.json
Outdated
| "command": "cody.manual-completions", | ||
| "title": "Cody: Generate completions" |
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.
The new title is better than the previous version but still a bit unclear, as it doesn't differentiate from inline completions. I'll need to execute this command to understand its functionality.
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.
Small nit, VS Code (and our ext) uses title case (https://title.sh) vs the usual Sourcegraph app style.
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.
I was expecting it to generate/show some completions in the editor file (like what I'm familiar with), and was surprised it opened in a new window. But I'd love to know more about this feature so I can help with naming… because even playing with it, I must admit I don't quite understand (someone plz save me!)
client/cody/package.json
Outdated
| "command": "cody.manual-completions", | ||
| "title": "Cody: Generate completions" |
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.
Small nit, VS Code (and our ext) uses title case (https://title.sh) vs the usual Sourcegraph app style.
| "title": "Cody: View Suggestions" | ||
| "command": "cody.manual-completions", | ||
| "title": "Cody: Generate completions" | ||
| }, |
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.
Not caused by this PR, but I discovered it when I was trying to test it locally.
I think we're missing a when clause here, because the command is always visible in the quickpick menu but only gets registered if the cody.experimental.suggestions setting is true. And you get an error if you try to use it from the quickpick.
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.
@toolmantim Fixed!
client/cody/package.json
Outdated
| "command": "cody.manual-completions", | ||
| "title": "Cody: Generate completions" |
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.
I was expecting it to generate/show some completions in the editor file (like what I'm familiar with), and was surprised it opened in a new window. But I'd love to know more about this feature so I can help with naming… because even playing with it, I must admit I don't quite understand (someone plz save me!)
Existing names are a bit confusing with the addition of inline multi-line completions, so I suggest that we: - Change the name we refer to for this feature to **Cody completions** as we discussed in Slack (I can look up the thread if you want) - Refer to the internal providers as _inline_ or _manual_. - Inline provider has support for multi-line completions - The action to trigger completions in the side bar is now called "Cody: Generate completions" ## Test plan <img width="924" alt="Screenshot 2023-05-23 at 14 42 01" src="https://github.com/sourcegraph/sourcegraph/assets/458591/8c690a3c-c9c4-4e74-b76b-e25d87051fc7"> <img width="1177" alt="Screenshot 2023-05-23 at 14 45 35" src="https://github.com/sourcegraph/sourcegraph/assets/458591/c1fc8635-bfb8-4600-81a1-bf9837ae05a6"> <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
Existing names are a bit confusing with the addition of inline multi-line completions, so I suggest that we:
Test plan