[Refactoring] Driver improvement for temp to instance variable #18821
Conversation
AlexisCnockaert
changed the title
| self refactoringError: temporaryVariableName , ' is a block parameter' ] | ||
| ] | ||
|
|
||
| { #category : 'as yet unclassified' } |
|
@AlexisCnockaert we should improve the class comments and the method comments too. |
| RBTemporaryToInstanceVariableRefactoring >> preconditionNoSubclassDefinesVar [ | ||
|
|
||
| ^ RBCondition | ||
| withBlock: [ (class allSubclasses anySatisfy: [ :cls | cls definesInstanceVariable: temporaryVariableName asString ]) not ] |
There was a problem hiding this comment.
In a second PR we should remove this pop up!
There was a problem hiding this comment.
In a second PR we should remove this pop up!
Yes, I suggested Alexis to leave it like this for the moment.
After merging this PR refactoring the method will be way easier
|
Why is it in draft mode? |
I agree with this, it dont have good comments which csn lead to confusion |
We worked on it with Carolina, it misses actions to work at 100% |
carolahp
left a comment
carolahp
left a comment
There was a problem hiding this comment.
@AlexisCnockaert I think we missed pushing the latest changes that include the methods for actions in the driver. You can ping me to take a second look ;)
| { #category : 'accessing' } | ||
| ReBrowseMethodsChoice >> description [ | ||
|
|
||
| ^ 'Browse the methods' |
There was a problem hiding this comment.
This description is almost the same as ReBrowseMethodChoice.
Maybe we could use an IV to set it from the driver and make it more specific?
| items: self breakingChoices; | ||
| display: [ :each | each description ]; | ||
| displayIcon: [ :each | self iconNamed: each systemIconName ]; | ||
| openModal |
There was a problem hiding this comment.
This code is duplicated in several driver.
We could move it up in the hierarchy in a future PR
There was a problem hiding this comment.
This is certainly a mistake; could you please revert it?
There was a problem hiding this comment.
Yes, this is happening when doing a merge from Iceberg :(
|
@carolahp actions are committed, sorry for the delay |
carolahp
left a comment
carolahp
left a comment
There was a problem hiding this comment.
This is great job!! We tested it live by video and also I checked the full code.
4 participants
It just needs to connect with actions and the driver will be good :)