-
Notifications
You must be signed in to change notification settings - Fork 213
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: Advanced Input #2884
Edit: Advanced Input #2884
Conversation
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 like the UI that shows me what will be edited by Cody, kind of reassuring 😃
I've left some comments in line, but the only issue i run into when trying this out is when using "cody.experimental.foldingRanges": "indentation-based"
, the expanded selection selected the whole class instead of the method itself:
Screen.Recording.2024-01-26.at.6.41.15.PM.mov
it's not related to this PR though, and other than that, I haven't run into other issues 👍
Can't wait to try it out when it's ready!
import type { GetItemsResult } from '../quick-pick' | ||
|
||
export const MODEL_ITEMS: Record<EditSupportedModels, vscode.QuickPickItem> = { | ||
'anthropic/claude-2.1': { |
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 won't be supported by enterprise users on older instances i believe?
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.
How do we handle this right now? I see we default to 2.0? Do you know why that is
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.
Iirc 2.1 is currently in preview, and I've seen some formatting issue with 2.1 response too.
I think 2.0 also does not work for all enterprise users though because some instances have other llm provider set up, so maybe it'd be better to use the ChatModelProvider to get the default model name of their instance unless they are on dotcom?
description: 'by Anthropic', | ||
alwaysShow: true, | ||
}, | ||
'anthropic/claude-instant-1.2': { |
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.
are these the only models we want to support for edit? or do we want to provide the same options the users have available in chat? if it's the later one, we can get the available model on user's instance via ChatModelProvider. wdyt?
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.
here is the list of the default models available if that's helpful: https://sourcegraph.com/github.com/sourcegraph/cody/-/blob/lib/shared/src/chat-models/index.ts?L5-48
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.
Hmmmm OK I'm looking at this now... might ping you if I have questions
@toolmantim @abeatrix I think this is ready for review now... I am adding some integration tests to cover the different scenarios that the quick pick can end up in. I am adding support for the "Model" selector in: #2951 After chatting with @abeatrix, we will need some extra work to ensure this works well with Enterprise and also shows the correct models. |
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.
Love the new quick pick area, super slick! I've left some inline comments with some additional questions:
- when trying to edit code at cursor position, it would add an empty line after the generated code, but i think the line should be added above the newly added code?
- Follow up from 1., adding an empty line above / below the current cursor won't work for edits that happened between existing code on the same line though 🤔 (don't think it should block this PR though)
- smart selection no longer work on
/doc
and/test
command when invoking from context menu or sidebar, is that intended?
@abeatrix Thanks for catching these bugs!
I think this is because I was incorrectly calculating the cursor position - just using the end of the selection. I've updated it so it actually uses
Yeah I think we might need to follow up on some more fine tuning for generate... The formatting can also be weird if you are within some existing code, I think maybe we need to use the full line in that case. That might help here too?
Nope, should be fixed now though! |
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.
Description
This PR adds an advanced Edit input that:
Document
command, and select a relevant symbol to documentTest
command, and select a relevant symbol to documentVideos
Changing range and model:
EditAdvancedRangeModel.mov
Document & Test
EditAdvancedDocumentTest.mov
Test plan
Create edits:
Modify the range of an edit:
Trigger the document command:
Trigger the unit test command: