Improve UI for rename argument/temporary variable and add tests#19846
Improve UI for rename argument/temporary variable and add tests#19846Biljcica wants to merge 7 commits into
Conversation
|
This issue has either a default title or empty body. We would appreciate it if you could provide more information. Note: I am not a very intelligent bot, I can only react to new comments. Please add a comment for me if you update the body or title. |
|
Thanks! Feel free to update description and explain what you did and optionally why! |
|
@Ducasse we want to leave this for Pharo15, right? |
This is my impression like that less stress :) |
|
I will change the branch when it is ready so that we have less git magic. |
balsa-sarenac
left a comment
balsa-sarenac
left a comment
There was a problem hiding this comment.
This is good progress, awesome to see tests alongside!
let's iterate and try to clean the noise, there are a lot of changes/additions that are not needed. I tried to flag them with inline comments. to get this merged, we ideally want minimal code to introduce, and remove the dead code/unused code. we can discuss if something is not clear
it would be nice if for next iteration to clean the leftover halts and new empty lines in random methods (just brings noise and unnecessary history)
| | temp | | ||
| temp := 35. | ||
| | tmp1 | | ||
| tmp1 := 35. | ||
|
|
||
| ^ temp | ||
| ^ tmp1 |
| { #category : 'initialization' } | ||
| ReCompositeChange >> resetChanges [ | ||
|
|
||
| changes := OrderedCollection new | ||
| ] | ||
|
|
There was a problem hiding this comment.
as discussed, let's try to do it with new model for each update, instead of cleaning the model before generating new changes
we can compare them afterwards and pick better one
| My precondition verifies that the new name is a valid variable name and not an existing instance or a class variable name | ||
| " | ||
| Class { | ||
| #name : 'ReRenameArgumentOrTemporaryRefactoring2', |
There was a problem hiding this comment.
What is different from original refactoring?
Or - what you needed to change and why? - could we use the existing refactoring instead?
| ] | ||
|
|
||
| { #category : 'accessing' } | ||
| ReRenameArgumentOrTemporaryDriver2 >> gatherUserInput [ |
There was a problem hiding this comment.
let's try to clean the driver of methods it is not using, just so it's easier to understand. I'm even thinking of creating a superclass of InteractionDriver (like a Driver), and have just the entrypoint and minimal things we need - like runRefactoring and hooks we use. we can discuss this, it will remove noise for you here, and since it's new way of doing drivers, doesn't make sense to try to force it and avoid all existing defined API
|
|
||
| " | ||
| Class { | ||
| #name : 'SycRefactoringPreviewPresenter2', |
There was a problem hiding this comment.
maybe rename this to SycRenameArgumentOrTemporaryPresenter or something similar, since this is now specific to this refactoring
3 participants
Copied StRefactoringPreviewPresenter and modified the UI to support renaming arguments/temporary variables. The updated view allows entering a new name, triggering the rename action, and displaying a preview of changes based on the selected scope. Added unit tests to verify each part of the functionality and introduced missing tests for ReLocalNameConflictCondition.