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

fix: Don't create intermediate variables when renaming a procedure argument. #8723

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Jan 7, 2025

The basics

The details

Resolves

Fixes #8693

Proposed Changes

This PR updates the procedure argument renaming logic to rename the variable being edited, rather than creating a new one for every keystroke and cleaning up the intermediate states when the field editor is dismissed. This prevents intermediate variables from appearing in the flyout when it is open while a procedure argument's name is being edited in the mutator.

@gonfunko gonfunko requested a review from a team as a code owner January 7, 2025 20:02
@gonfunko gonfunko requested a review from cpcallen January 7, 2025 20:02
@github-actions github-actions bot added the PR: fix Fixes a bug label Jan 7, 2025
@rachel-fenichel
Copy link
Collaborator

Christopher said to also check one edge case: "When checking any fix for this, do test (or better still: add tests) to see what happens if one of the intermediate variable names clashes with another variable/parameter—e.g., if there is a variable foo and you rename a parameter from f to food."

Did you check it, and does it work?

@gonfunko
Copy link
Contributor Author

gonfunko commented Jan 7, 2025

Yes and yes; behavior appears identical to the old version in that case. The pre-existing variable does not get deleted if you temporarily land on it but continue past it, and if you name the arg the same they coalesce.

Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

This looks good to me--I like that you are actually using the variable object to make sure that you can just change the active variable, instead of creating a pile of them and then going back to delete.

We have several versions of the procedure blocks now--particularly in block-plus-minus and block-shareable-procedures. Please check whether you can apply the same fix there, and either apply it or file bugs to track that work.

@gonfunko gonfunko merged commit 4dcffa0 into google:rc/v12.0.0 Jan 8, 2025
11 checks passed
@gonfunko gonfunko deleted the intermediate-vars branch January 8, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants