-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cody: toggle config for inline and non-stop no reload required #53348
Conversation
this.contextStore.set(id, { docUri, original, replacement }) | ||
} | ||
|
||
public async undo(id: string): Promise<void> { |
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.
Clicking on undo will replace the replacement context with the original context
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.
- We shouldn't add a "revert" to the code lens, it seems unrelated. The lenses have a scope within the file; revert applies over the whole file.
- Don't make non-stop fixup dynamic in this way. This needs care to handle in-flight work work shutting down cleanly. I don't think it's worth it because it will complicate respinning, etc. but if we want to do it, it needs tests and probably more work.
- Up to you about inline chat but do similar concerns apply?
const isTesting = process.env.CODY_TESTING === 'true' | ||
if (e.affectsConfiguration('cody')) { | ||
// Non-Stop Cody | ||
const enableNonStop = (config.get('experimental.nonStop') as boolean) || isTesting |
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.
Let's not make non-stop fixups configurable at runtime. There's work to cleanly shut down things in flight if you can toggle this dynamically that isn't being attempted here. So please back this part of the change out.
Inline assist, I leave to you judgement, but you should test cases like having an inline assist spinning and then turning the feature off; having it on, spin, off, on again, etc.
On the whole I don't think this is worth the complexity it might entail. Do people want to flip the availability of these features off and on a lot?
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.
Yea I will remove Non-Stop Cody from this.
Do people want to flip the availability of these features off and on a lot?
I think we just want to make it easier so that users do need to reload their IDE when they enable the feature.
@dominiccooney updated based on your last review
|
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.
Looks good.
Would be great to spell out the command to run the existing tests in the test plan. It will help others grok how to run those tests.
Requested by #53307
Close #53351 #53363
This PR includes:
For details: https://www.loom.com/share/540f4953b1ac4c9d81e90f4c37f86c2d?sid=41c65fd7-1be2-4225-92eb-3c7fa56766f8
Test plan
All the existing tests should be passing.
Cody_.Toggle.configs.for.Inline.Assist.and.Non-Stop.Cody.mp4