[Refactorings] Implement Driver for PushDownSharedVariable #18600
Conversation
carolahp
changed the title
| RePushDownSharedVariableRefactoring >> preconditions [ | ||
|
|
||
| ^ self applicabilityPreconditions & self breakingChangePreconditions | ||
| ] |
There was a problem hiding this comment.
This should not be needed anymore, and should not be used anywhere. I think this will also fail if it is called because both calls return lists and we try to & them. I should probably do a cleanup and remove preconditions soon
| { #category : 'accessing' } | ||
| ReVariableCondition >> violatorCandidates [ | ||
|
|
||
| self subclassResponsibility | ||
| ] |
There was a problem hiding this comment.
What is the purpose of this new method? I saw it's usage, but was wondering what the existing API was missing?
There was a problem hiding this comment.
violatorCandidates is the same as subjects in this PR. It is necessary to improve the error messages of condition negations. I think the implementation in the mentioned PR is more clean. We may review and merge that PR before this one
| { #category : 'accessing' } | ||
| ReChoice >> action [ | ||
| "The action is dynamically set as a block. Some subclasses may override this method to hardcode their own actions" | ||
| ^ actionBlock value | ||
| ] | ||
|
|
||
| { #category : 'accessing' } | ||
| ReChoice >> actionBlock: aBlockClosure [ | ||
| actionBlock := aBlockClosure | ||
| ] | ||
|
|
There was a problem hiding this comment.
What is the upside of this approach compared to the existing approach?
Right now, we just invoke proper methods from the driver:
driver applyChanges.
driver browseSenders.
I'm asking because there is two ways to do 1 thing, and it would be better to only be one way. So either this or old one. If you want this all usages should be changed so we don't have confusion...
There was a problem hiding this comment.
I see, I didn't understand the standard way to do it when I prepared this PR. I will change the code to the existing approach
| ^ self class defaultSelectDialog | ||
| title: 'Warning you may break your code'; | ||
| label: | ||
| 'The variable you want to push down is referenced from methods in its defining class!'; |
There was a problem hiding this comment.
Would it make sense to ask breaking change for its violation message?
hasNoReferences violationMessage
that way we don't have to hard-code anything
There was a problem hiding this comment.
Yes! it makes sense, removed the hardcoded text
| RePushDownVariableDriver >> configureRefactoring [ | ||
|
|
||
| " we should manage multiple instance variables" | ||
|
|
||
| refactoring := self refactoringClass | ||
| model: model | ||
| variable: variable | ||
| class: class | ||
| ] |
There was a problem hiding this comment.
self prepareForExecution at the end
|
Thanks Balsa. I'm thinking that it would be great if you have a list of constraints that the implementation should follow. I could try to turn them into automated rules. |
|
@Ducasse we kind of have that written here: https://github.com/pharo-project/reEngine/blob/master/DEVELOPMENT.md
|
|
@carolahp did you see the feedback for balsa? |
3 participants
Also refactor variable-related choices.
Finally, refactors conditions to optionally set their action as a block from the driver (preventing the creation of a new class each time)