New driver for the push up method refactoring (P13) #16637
Conversation
|
Hi caro this is nice to have you back on that. |
…ot all choices are handled yet
|
Hi @carolahp let us know when you want a review. |
|
Hi @Ducasse and @balsa-sarenac , I would appreciate your review on this PR. |
|
There are lots of failing tests: |
|
Related breaking tests are due to modifications to |
|
@carolahp I would like to understand the difference between an unfixable case and a precondition that is violated. |
@Ducasse the only difference is that an unfixable case is associated with a choice, for example "browse overriden method". However, I didn't think before that it may be simpler to just make these "unfixable cases" into preconditions. I will update the PR to do this |
There was a problem hiding this comment.
Hey Caro, great work!
For the question on "fixable" vs "unfixable". I agree with Steph that "unfixable"s are basically applicability preconditions. And for breaking changes, we know this from the beginning that some of them can be fixed by other refactoring (what you refer to as "fixable") and others are just warnings (the refactoring cannot promise behaviour preservation and there are no fixes it can offer either). We didn't had them so far in the architecture since there was not a need to differentiate them. Maybe they can just be methods on preconditions, that if a precondition fail you can ask it what is the refactoring that fixes you? However, I see a drawback here with reusability (multiple refactorings use the same precondition and they want different fixes when they fail, or for one refactoring there can be breaking change and for other is applicability). Let me know how did you planned to integrate this in the current architecture?
| { #category : 'accessing' } | ||
| RBClass >> methodNamed: aSymbol [ | ||
|
|
||
| | allMethods | | ||
| allMethods := IdentitySet new. | ||
| self methods do: [ :each | each selector = aSymbol ifTrue: [^ each]]. | ||
| ^ nil | ||
| ] | ||
|
|
There was a problem hiding this comment.
There already exists a method for this: methodFor:. Could you remove this one?
| { #category : 'accessing - methods' } | ||
| RBMetaclass >> methodNamed: aSymbol [ | ||
|
|
||
| | allMethods | | ||
|
|
||
| allMethods := IdentitySet new. | ||
| self allMethods do: [ :each | each selector = aSymbol ifTrue: [^ each]]. | ||
| ^ nil | ||
| ] | ||
|
|
| { #category : 'accessing' } | ||
| RBMetaclass >> classVariableNames [ | ||
| ^ self instanceSide classVariableNames | ||
| ] | ||
|
|
There was a problem hiding this comment.
I'm curious, what is the use case when you use this?
There was a problem hiding this comment.
The only use case is in the RBPullUpMethodParametrizedTest>>testFailurePullUpClassMethod , where the refactoring should fail
| { #category : 'preconditions' } | ||
| RBPullUpMethodRefactoring >> pushUpSharedVariable: aVariable [ | ||
| | refactoring | | ||
| refactoring := RBPullUpClassVariableRefactoring | ||
| model: self model | ||
| variable: aVariable | ||
| class: targetSuperclass. | ||
| self generateChangesFor: refactoring | ||
| ] |
There was a problem hiding this comment.
Would it make sense to rename this method to be inline with the rest of the methods like: pullUpSharedVariable:?
There was a problem hiding this comment.
I think yes, do you think we should also rename the method RBPullUpMethodRefactoring >> pushUpVariable:?
| RBPullUpMethodRefactoring >> removeDuplicatesOf: aSelector [ | ||
|
|
||
| | tree | | ||
| tree := targetSuperclass parseTreeForSelector: aSelector. | ||
| targetSuperclass allSubclasses do: | ||
| [:each | | ||
| ((each directlyDefinesMethod: aSelector) and: | ||
| [(tree equalTo: (each parseTreeForSelector: aSelector) exceptForVariables: #()) | ||
| and: [(each superclass whoDefinesMethod: aSelector) == targetSuperclass]]) | ||
| ifTrue: | ||
| [removeDuplicates | ||
| ifFalse: | ||
| [removeDuplicates := | ||
| self | ||
| refactoringConfirmWarning: 'Do you want to remove duplicate subclass methods?']. | ||
| removeDuplicates ifTrue:[ | ||
| self generateChangesFor: | ||
| (RBRemoveMethodTransformation | ||
| selector: aSelector from: each)]]] | ||
| targetSuperclass allSubclasses do: [ :each | | ||
| ((each directlyDefinesMethod: aSelector) and: [ | ||
| (tree | ||
| equalTo: (each parseTreeForSelector: aSelector) | ||
| exceptForVariables: #( )) and: [ | ||
| (each superclass whichClassIncludesSelector: aSelector) | ||
| == targetSuperclass ] ]) ifTrue: [ | ||
| removeDuplicates ifFalse: [ | ||
| removeDuplicates := self refactoringConfirmWarning: | ||
| 'Do you want to remove duplicate subclass methods?' ]. | ||
| removeDuplicates ifTrue: [ | ||
| self generateChangesFor: | ||
| (RBRemoveMethodTransformation selector: aSelector from: each) ] ] ] |
There was a problem hiding this comment.
I'm thinking that this method should be moved to the driver?
There was a problem hiding this comment.
You are right, I created a new condition called ReMethodsHaveNoDuplicatesCondition that encapsulates this functionality
There was a problem hiding this comment.
But this is not just suggestion, this is a cleanup action that will remove methods with duplicate body in all siblings. So you may need a condition, but you also need to be able to call this transformation as well. This can be done along with "Push up referenced instance variables"?
| RBPullUpMethodRefactoring >> applicabilityPreconditions [ | ||
| " Check that all selectors are defined in `class`, | ||
| and that `class` is in the hierarchy of subclasses of `targetSuperclass` " | ||
|
|
||
| ^ { | ||
| (RBCondition hasSuperclass: class). | ||
| (ReDefinesSelectorsCondition new | ||
| definesSelectors: selectors | ||
| in: class). | ||
| (RBCondition withBlock: [ | ||
| self checkClassVars. | ||
| self checkSuperclass. | ||
| self checkSuperMessages. | ||
| true ]) } | ||
| (ReClassHasSubclassesCondition new | ||
| class: targetSuperclass; | ||
| subclassesList: { class name }). | ||
| self preconditionNoOverrides. | ||
| self preconditionNoSupersendsSent. | ||
| self preconditionNoSupersendsReceived } | ||
| ] |
There was a problem hiding this comment.
I think that we here miss calls to preconditionsNoReferencesToInstVars and preconditionsNoReferencesToSharedVars? Can you double check that all previous preconditions are still called in the new version after the refactoring as well?
| { #category : 'execution' } | ||
| RePushUpMethodDriver >> breakingChoices [ | ||
|
|
||
| ^ refactoring failedBreakingChangePreconditions collect: [ :each | | ||
| each class strategyChoiceClass new driver: self ] | ||
| ] |
There was a problem hiding this comment.
This is cool! However, we should support multiple choices per breaking change preconditions. This can be dealt with in future PR, not this one.
| | presenter changes | | ||
|
|
||
| "Display previous transformations that will fix the fixable breaking conditions" | ||
| changes := self breakingChoices. |
There was a problem hiding this comment.
The variable name should be choices right?
| { #category : 'action' } | ||
| RePushUpMethodDriver >> pushUpReferencedInstVars [ | ||
|
|
||
| notInstVarRefs violators do: [ :violator | | ||
| "We add the pushUpVariable transformation to the refactoring previous transformations" | ||
| refactoring pushUpVariable: (violator at: 2) ] | ||
| ] |
There was a problem hiding this comment.
Would it make sense here to encapsulate this violator at: 2? Something like notInstVarRefs referencedInstanceVariables do: [ :iv | refactoring pushUpVariable: iv ]
| RePushUpMethodDriver >> pushUpReferencedSharedVars [ | ||
|
|
||
| notSharedVarRefs violators do: [ :violator | | ||
| "We add the pushUpVariable transformation to the refactoring previous transformations" | ||
| refactoring pushUpSharedVariable: (violator at: 2) ] | ||
| ] |
There was a problem hiding this comment.
Same comment as for inst vars
Ducasse
added
the
Status: Need more work
Summary
Implements driver for handling the user interaction logic when applying the push up method refactoring.
Tests are created for the new ReConditions
Replaces PR#16438
Depends on Spec PR#1548
About the architecture