ExtractTempDriver #17168

SandraSamardzic · 2024-09-27T00:22:17Z

Creating an interaction driver for RBExtractToTemporaryRefactoring

Ducasse · 2024-09-29T07:41:21Z

Thanks a lot!

Ducasse · 2024-09-29T07:41:32Z

I will try to find some time to have a look.

SandraSamardzic · 2024-09-29T20:14:22Z

Okay, thank you @Ducasse, I appreciate it.

balsa-sarenac

Thanks Sandra, this looks good on quick skim. To make this complete you need to make extract temp command actually use this driver.

Find the command class and do required changes to make it use driver instead of refactoring
Migrate any UI from command to driver
Create at least a happy path test for this driver, if already done, create a test with invalid selection or invalid variable name

… insted of the refactroing class

…e driver

balsa-sarenac

Really nice work, left some minor comments, nothing too huge. Well done!

balsa-sarenac · 2024-10-05T08:37:20Z

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

+
+    self setUpDriver: driver.
+
+    self should: [ driver runRefactoring ] raise: Error.


It would be nice if we would expect as specific error as possible. I'm guessing this is RBRefactoringError?

balsa-sarenac · 2024-10-05T08:37:41Z

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

+
+    self setUpDriver: driver.
+
+    self should: [ driver runRefactoring ] raise: Error.


Same comment as above

balsa-sarenac · 2024-10-05T08:41:17Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+			firstCondition check ifFalse: [
+				self setErrorTextFromCondition: firstCondition onPresenter: presenter ].
+			secondCondition check ifFalse: [ 
+				self setErrorTextFromCondition: secondCondition onPresenter: presenter ].


If they both fail, it will only show the error text from the second one, right?

balsa-sarenac · 2024-10-05T08:49:03Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+{ #category : 'execution' }
+ReExtractToTemporaryDriver >> requestSelector [
+
+	^ self requestUserInput: 'Please provide a selector'
+			 label: 'The selector should be defined.'  
+			 validate: [ :string :presenter | self validateSelector: string onPresenter: presenter ]
+]


Two things here, both are non-blocking:

since Driver should only be called through UI, chances that selector is invalid are close to 0, so this check is optional in the driver and will always pass

correct way of asking user for selector would be to use selection presenter that you fill with available selectors from the class, but again this is not needed since this will be correctly set always

This check is important when we are invoking this refactoring programatically, and it should then raise an exception, and not ask for new selector.

My suggestion might be to remove this code from driver. Would like to hear you thoughts as well @Ducasse

balsa-sarenac · 2024-10-05T08:51:23Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+ReExtractToTemporaryDriver >> validateName: aName onPresenter: presenter [
+
+	| firstCondition secondCondition result |
+	firstCondition := ReIsValidInstanceVariableName new name: aName.


I would suggest to ask refactoring for preconditions like: refactoring preconditionValidInstVarName and refactoring preconditionCheckVariableName that way we can reduce duplication and avoid potential misconfiguration.

balsa-sarenac · 2024-10-05T08:55:53Z

src/SystemCommands-SourceCodeCommands/SycExtractTempCommand.class.st

 SycExtractTempCommand >> asRefactorings [

-	^ {RBExtractToTemporaryRefactoring
-		  extract: sourceNode sourceInterval
-		  to: tempName
-		  from: method selector
-		  in: method origin}
+	| driver |
+	driver := ReExtractToTemporaryDriver new.
+	driver 
+		extract: sourceNode sourceInterval
+	 	to: tempName
+	 	from: method selector
+	 	in: method origin.
+
+	^ driver runRefactoring.
+
 ]


Take a look at how were other drivers configured to run here. This does work, but there is a slightly better way:

override isComplexRefactoring to return false

this method is unused then

override execute with this code
Take a look at SycRemoveClassCommand. I'm saying this since asRefactoring and isComplexRefactoring will be removed in the future and this will then not work.

…o provide better understand and woking tests. Removeing unsed or methods that are not needed.

SandraSamardzic · 2024-10-05T19:20:38Z

I deleted the dialog related to the selector as you suggested @balsa-sarenac , before I didn't consider that there might not be a need for it, my mistake. If @Ducasse thinks otherwise, it's not a problem for me to add it again and change it so that it's an optional dialog.

src/Refactoring-Core/RBExtractToTemporaryRefactoring.class.st

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

src/SystemCommands-SourceCodeCommands/SycExtractTempCommand.class.st

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

balsa-sarenac · 2024-10-05T20:40:49Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+{ #category : 'execution' }
+ReExtractToTemporaryDriver >> requestVariableName [
+
+	newVariableName := self defaultRequestDialog


I think that source of your errors are the mocks. They are very tricky. Basically here we use defaultRequestDialog. We send it message title:, label: and validateAnswer:. We need mock for it that will answer for example self for first two, and on validateAnswer: it will answer newVariableName that we want.

balsa-sarenac · 2024-10-05T20:42:32Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+]
+
+{ #category : 'accessing' }
+ReExtractToTemporaryDriver >> variableNameEditorPresenterClass [


Maybe this function's name should be defaultRequestDialog? Can you check how rename method did this mocking, might be helpful. If needed we can pair on this.

I used rename method as an example, it has methodNameEditorPresenterClass method. Rename method uses a requestDialogWith: and I just made a default one, I saw an example of it in the rename class refactoring. Now I see that my focus should be there, thank you @balsa-sarenac

I thought so, thats why I asked if you need both mocks. If you need help, reach out. Try to debug rename method to see exactly how was mock set up, and then with that knowledge create mock specific to this use-case.

balsa-sarenac · 2024-10-05T20:48:45Z

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

+    dialog := MockObject new.
+
+    dialog 
+        on: #variableName
+        respond: (RBArgumentName name: 'tempVar').
+
+    dialog
+        on: #variableName
+        respond: (RBArgumentName name: #anotherVar).
+
+    driver requestDialog: dialog.


We craft these mocks based on how they are used in the driver. I'm not sure they are configured correctly. Basically what we do is create mock object. Go in the class and see exactly which messages are sent to this object. Each of them define here in on:respond:. Pay attention which message should return something meaningful, and which ones you don't care (if you don't care, return self).
For example requestDialog: receives name:, label: and validateAnswer:, right?

Main disadvantage of mocks is that they are tightly coupled to implementation and I'm not a big fan of them, but this is only way we found to test drivers.

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

…ons. Removing a method that is not used anymore, fixing validation messages.

… Creating a new dialog

…ng needed changes

balsa-sarenac

Really nice work and improvement over last time!

Pay attention to these:

CI fails because this method is never called StVariableNameEditorPresenter>>#renameVariableAndClose:
there are two failing driver tests (first, second)

src/Refactoring-UI-Tests/ReExtractToTemporaryDriverTest.class.st

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

src/Refactoring-UI/StVariableNameEditorPresenter.class.st

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

balsa-sarenac · 2024-10-14T12:27:39Z

src/Refactoring-UI/ReExtractToTemporaryDriver.class.st

+	newVariableName := self requestVariableName.
+	refactoring extract: sourceInterval to: newVariableName from: selector in: class.
+	shouldEscape ifTrue: [ ^ self ].
+	refactoring preconditionValidInstVarName check ifFalse: [ ^ self ].
+	refactoring preconditionCheckVariableName check ifFalse: [ ^ self ].


Looks like you don't have while false here anymore, and you are still using shouldEscape, is this on purpose? What happens when user types wrong name, do we offer it to give us new name with a helpful error message?

src/Refactoring-UI/StVariableNameEditorPresenter.class.st

src/SystemCommands-SourceCodeCommands/SycExtractTempCommand.class.st

…ts on StVariableNameEditorPresenterClass. Updaing code

…iting presenter to work and look better. Editing method names and lebel texts

Ducasse · 2024-10-18T14:05:58Z

nearly there!

Ducasse · 2024-10-18T14:07:22Z

testFailureInvalidVariableNameWhenVariableIsAlreadyTemporary is failing

Ducasse · 2024-10-20T09:02:26Z

I could also integrate and skip that test. Let me know

SandraSamardzic · 2024-10-20T10:01:39Z

@Ducasse the dialog is not working very well, so my focus is on that now. I will let you know when I fix the dialog so you can integrate it then we will see what is wrong with the tests

Ducasse · 2024-10-21T14:01:08Z

Super. Keep us up to date.

Ducasse · 2024-11-03T16:14:11Z

Hello do you have some update?

SandraSamardzic · 2024-11-04T02:23:23Z

Hello @Ducasse, unfortunately I have been caught up with other commitments, which has slowed down my pace. Thankfully, these other obligations are getting resolved, and I hope that I will fix the dialog by Thursday. I apologize to keep you waiting.

…efactoring.

SandraSamardzic · 2024-11-07T20:54:28Z

I am sorry @Ducasse , I still haven't been able to fix the problems. There are two problems :

no error message is printed in the dialog when the precondition is not met,
refactoring does not make any changes.

It is possible that there are some other mistakes that I did not see. I don't do well with the presenter, not sure if the presenter is the problem or runRefactoring method. I will contact @balsa-sarenac , if he has time to take a look at the problems, maybe I should change the approach to the dialog. Again, apologies for the slow work.

Ducasse · 2024-11-09T21:24:07Z

Ok no problem.
What is the problem? Because may be we can merge and fix the rest in another PR?

SandraSamardzic · 2024-11-11T04:19:28Z

Maybe it is possible, but I am not sure how big the problems are @Ducasse. When the refactoring is started, a dialog appears, which is created using the presenter class, if the precondition is not met, an error message should be displayed in the dialog, I set the error message to a label in the presenter but I am not sure how to print it. This is the first problem, the second problem is when the correct variable is entered and the refactoring should happen there are no changes, the variable should be created and have the value of the selected interval but that is not the case, I don't know why.

Ducasse · 2024-11-22T12:39:11Z

Ok @balsa-sarenac do you have some cycles for this?
Else we can integrate it and see later because I'm afraid that further changes will impact it.

Ducasse · 2024-11-26T12:56:23Z

@balsa-sarenac when I integrated this PR it broke the build

Error: Package Refactoring-UI depends on the following classes:
RBInteractionDriver
You must resolve these dependencies before you will be able to load these definitions:
ReExtractToTemporaryDriver
ReExtractToTemporaryDriver>>#changes

I will add RBInteractionDriver to be able to load it.

Creating a driver for extract to temporary refactoring

d2c06ac

Ducasse requested a review from balsa-sarenac September 29, 2024 07:41

balsa-sarenac reviewed Sep 30, 2024

View reviewed changes

SandraSamardzic added 2 commits September 30, 2024 16:14

Adding a method for initialization in the driver and using the driver…

c76e5cb

… insted of the refactroing class

Changes in refactoring and driver class. Creating a test class for th…

e076f26

…e driver

balsa-sarenac suggested changes Oct 5, 2024

View reviewed changes

SandraSamardzic added 3 commits October 5, 2024 18:37

Added methods to refactoring for accesing. Adding methods to driver t…

f4d1a00

…o provide better understand and woking tests. Removeing unsed or methods that are not needed.

Unused method

b7f1239

Adding an instanceVariable

0494872

balsa-sarenac reviewed Oct 5, 2024

View reviewed changes

SandraSamardzic added 7 commits October 6, 2024 00:56

Using new methods in the refactroing class in applicabilityPreconditi…

5c8af00

…ons. Removing a method that is not used anymore, fixing validation messages.

Removing unnecessary methods and lines

f9d6933

Removing model: , initializing the model variables in scopes: method.…

148576c

… Creating a new dialog

fixes in methods for dialog set up

222fac6

Making a presnter class for extract to temporary refactoring and maki…

79ded09

…ng needed changes

autofix categories

f18cfea

Fixing mistakes from the last commit

a53d469

balsa-sarenac reviewed Oct 14, 2024

View reviewed changes

SandraSamardzic added 4 commits October 14, 2024 23:25

Deleting methods that are not used. Renameing a test. Updating commen…

b033351

…ts on StVariableNameEditorPresenterClass. Updaing code

Removing unused methods. Removing unneeded dialog. Fixing issues. Ed…

476f83a

…iting presenter to work and look better. Editing method names and lebel texts

printing preconditions errors

188dd06

looping the dialog

0a24ec3

Ducasse added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Oct 21, 2024

Remooving false comment. Fixing the dialog for extract to temporary r…

d19a747

…efactoring.

Ducasse merged commit c1f5410 into pharo-project:Pharo13 Nov 25, 2024
1 of 2 checks passed

Ducasse mentioned this pull request Nov 26, 2024

Revert "ExtractTempDriver" #17452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExtractTempDriver #17168

ExtractTempDriver #17168

SandraSamardzic commented Sep 27, 2024

Ducasse commented Sep 29, 2024

Ducasse commented Sep 29, 2024

SandraSamardzic commented Sep 29, 2024

balsa-sarenac left a comment

balsa-sarenac left a comment

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

SandraSamardzic commented Oct 5, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac Oct 5, 2024

SandraSamardzic Oct 5, 2024

balsa-sarenac Oct 6, 2024

balsa-sarenac Oct 5, 2024

balsa-sarenac left a comment

balsa-sarenac Oct 14, 2024

Ducasse commented Oct 18, 2024

Ducasse commented Oct 18, 2024

Ducasse commented Oct 20, 2024

SandraSamardzic commented Oct 20, 2024

Ducasse commented Oct 21, 2024

Ducasse commented Nov 3, 2024

SandraSamardzic commented Nov 4, 2024 •

edited

Loading

SandraSamardzic commented Nov 7, 2024

Ducasse commented Nov 9, 2024

SandraSamardzic commented Nov 11, 2024 •

edited

Loading

Ducasse commented Nov 22, 2024

Ducasse commented Nov 26, 2024


		self setUpDriver: driver.

		self should: [ driver runRefactoring ] raise: Error.

ExtractTempDriver #17168

ExtractTempDriver #17168

Conversation

SandraSamardzic commented Sep 27, 2024

Ducasse commented Sep 29, 2024

Ducasse commented Sep 29, 2024

SandraSamardzic commented Sep 29, 2024

balsa-sarenac left a comment

Choose a reason for hiding this comment

balsa-sarenac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandraSamardzic commented Oct 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balsa-sarenac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ducasse commented Oct 18, 2024

Ducasse commented Oct 18, 2024

Ducasse commented Oct 20, 2024

SandraSamardzic commented Oct 20, 2024

Ducasse commented Oct 21, 2024

Ducasse commented Nov 3, 2024

SandraSamardzic commented Nov 4, 2024 • edited Loading

SandraSamardzic commented Nov 7, 2024

Ducasse commented Nov 9, 2024

SandraSamardzic commented Nov 11, 2024 • edited Loading

Ducasse commented Nov 22, 2024

Ducasse commented Nov 26, 2024

SandraSamardzic commented Nov 4, 2024 •

edited

Loading

SandraSamardzic commented Nov 11, 2024 •

edited

Loading