Skip to content
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

Update the FixUp retry instruction text #1615

Merged
merged 9 commits into from
Nov 6, 2023
Merged

Update the FixUp retry instruction text #1615

merged 9 commits into from
Nov 6, 2023

Conversation

deepak2431
Copy link
Contributor

@deepak2431 deepak2431 commented Nov 2, 2023

The retry instruction text has the command attached, which recursively adds to each retry. This PR fixes this issue that in case of any fixup task, only the previous instruction is used/shown in the quick pick.

Test plan

  • Tested manually.

Before behaviour:

REC-20231102230135.mp4

After:

REC-20231102230014.mp4

@deepak2431
Copy link
Contributor Author

@umpox @abeatrix Can I have a review, please? Just a small bug fix which I found while playing around the FixUp.

@@ -877,7 +877,7 @@ export class FixupController
return
}
const previousRange = task.selectionRange
const previousInstruction = task.instruction
const previousInstruction = task.instruction.replace(/^\/edit/, '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const previousInstruction = task.instruction.replace(/^\/edit/, '')
const previousInstruction = task.instruction.replace(/^\/edit /, '')

Good catch! I'm thinking maybe instead of the retry step, we should do this during the FixupTask creation step to make sure /edit is removed for all tasks, what do you think? @deepak2431 @toolmantim

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what the trade off is here, but trust your call @abeatrix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abeatrix Agree with you, that would be good to do I think so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh woops I meant to tag Tom @umpox here, sorry @toolmantim lol

@@ -25,6 +25,7 @@ Starting from `0.2.0`, Cody is using `major.EVEN_NUMBER.patch` for release versi
- Commands: Commands selector in chat will now scroll to the selected item's viewport automatically. [pull/1556](https://github.com/sourcegraph/cody/pull/1556)
- Edit: Errors are now shown separately to incoming edits, and will not be applied to the document. [pull/1376](https://github.com/sourcegraph/cody/pull/1376)
- Chat: Prevent cursor from moving during chat command selection. [pull/1592](https://github.com/sourcegraph/cody/pull/1592)
- Fixup: Updated the retry instruction to just use the previous command text. [pull/1615](https://github.com/sourcegraph/cody/pull/1615)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just made a patch release so you might want to move it up to the new empty section after merging with main 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah saw it. I will update the changelog. No issues!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @abeatrix Please check your Discord.

Comment on lines 152 to 153
const fixupInstruction = instruction.replace(/^\/edit/, '').trim()
const task = new FixupTask(fixupFile, fixupInstruction, selectionRange, insertMode, source)
Copy link
Contributor

@umpox umpox Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak2431 Can we do this in the FixupTask constructor instead? Just so we don't accidentally miss this in future

    constructor(
        public readonly fixupFile: FixupFile,
        public readonly instruction: string,
        public selectionRange: vscode.Range,
        // insert mode means insert replacement at selection, otherwise replace selection contents with replacement
        public insertMode?: boolean,
        // the source of the instruction, e.g. 'code-action', 'doc', etc
        public source?: ChatEventSource
    ) {
        this.id = Date.now().toString(36).replaceAll(/\d+/g, '')
        this.instruction = instruction.replace(/^\/edit/, '').trim()
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umpox Yeah, that will be even better. Let me make those changes.

@umpox
Copy link
Contributor

umpox commented Nov 3, 2023

Thanks! Left a small comment but looks good!

@deepak2431
Copy link
Contributor Author

@umpox Made the requested changes. I think it's all good now.

@deepak2431 deepak2431 requested a review from umpox November 3, 2023 14:04
Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @deepak2431!

@@ -19,6 +19,7 @@ Starting from `0.2.0`, Cody is using `major.EVEN_NUMBER.patch` for release versi
### Fixed

- Chat: Fixed an issue where multiple action buttons were appended to each Code Block per chat message. [pull/1617](https://github.com/sourcegraph/cody/pull/1617)
- Fixup: Updated the fixup create task to just use the previous command text. [pull/1615](https://github.com/sourcegraph/cody/pull/1615)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still showing up in the old version 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it now. My bad 😅

@umpox umpox enabled auto-merge (squash) November 6, 2023 09:36
@umpox umpox disabled auto-merge November 6, 2023 10:02
@umpox umpox enabled auto-merge (squash) November 6, 2023 10:08
@umpox umpox merged commit a057cf5 into sourcegraph:main Nov 6, 2023
23 of 25 checks passed
@deepak2431 deepak2431 deleted the deepak/fixup-retry branch November 29, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants