pharo icon indicating copy to clipboard operation
pharo copied to clipboard

[RB] Pull up does not push up instance variables when asked to

Open Ducasse opened this issue 8 months ago • 9 comments

Imagine you want to pull up a method and this method access an instance variable. Pull up will ask you if you want to pull up the instance variable too. If you confirm that you want to do it. The refactoring does not perform it.

Ducasse avatar Jun 15 '25 20:06 Ducasse

May be because the case I have is on class side.

Ducasse avatar Jun 15 '25 20:06 Ducasse

I will investigate further

Ducasse avatar Jun 15 '25 20:06 Ducasse

So this is not working on instance side or class side. Now I checked and the push up instance variable is working so this is a problem of the driver of PullUp. The action (in addition push up the variable) is calling the refactoring but it looks like the changes are not added to the list.

Ducasse avatar Jun 16 '25 08:06 Ducasse

@balsa-sarenac do you have an idea?

Ducasse avatar Jun 16 '25 08:06 Ducasse

The action calls:

Driver >> pullUpReferencedInstVars

	notInstVarRefs referencedInstanceVariables do: [ :instVar | 
		"We add the pushUpVariable transformation to the refactoring previous transformations"
		refactoring pullUpVariable: instVar ]

Which calls:

RePullUp .... >> pullUpVariable: aVariable
	| refactoring |
	refactoring :=  RePullUpInstanceVariableRefactoring
			model: self model
			variable: aVariable
			class: targetSuperclass.
	self generateChangesFor: refactoring

Ducasse avatar Jun 16 '25 10:06 Ducasse

In Driver we have

Driver>>applyChanges

	| applied |
	applied := self openPreviewWithChanges: self changes.

and changes

changes
	"Drivers should not call generateChanges of Refactoring because generateChanges is performing the preconditions and raising errors 
	The API method generateChanges is for refactoring scripting."
	
	refactoring privateTransform.
	^ refactoring changes

I'm changing the pullUpReferencedInstVars to use a compositeChange

Ducasse avatar Jun 16 '25 11:06 Ducasse

I tried

pullUpReferencedInstVars

	| compositeChange |
	compositeChange := RBCompositeRefactoryChange new.
	compositeChange name: 'pulled up instances'.
	self halt.
	( notInstVarRefs referencedInstanceVariables do: [ :instVar | 
		"We add the pushUpVariable refactoring to the refactoring previous transformations."
		self flag: #stef. 
		"should we not call privateTransform instead of generateChanges"
		compositeChange addChange: (refactoring pullUpVariable: instVar) generateChanges ]).
	
	self openPreviewWithChanges: compositeChange

but it only pull up the method then and I would like to have also the method pulled up.

Ducasse avatar Jun 16 '25 11:06 Ducasse

I changed it back and change the flow of the driver but now I get method push to siblings :(

configureAndRunRefactoring

	self configureRefactoring.

	"Pay attention that the applicability preconditions do  not include checking the superclass for a 
	method with the same name because during an interaction we when to let the user the possibility to 
	do actions to handle this specific case. This is why similarly method in direct superclass is checked 
	but in the breaking change precondtions."
	refactoring failedApplicabilityPreconditions ifNotEmpty: [
		^ self alertFailedPreconditions ].
	
	self setBreakingChangesPreconditions.
	refactoring failedBreakingChangePreconditions
		ifEmpty: [ self applyChanges ]
		ifNotEmpty: [ self handleBreakingChanges ; applyChanges ]
		
	"applying the changes blindly after handleBreakingChanges is not 
	good because the user may have press cancel. 
	and in such a case we should cancel the complete flow."

Ducasse avatar Jun 16 '25 12:06 Ducasse

This is normal. Now I got the solution. PR on its way.

Ducasse avatar Jun 16 '25 12:06 Ducasse