Merge pull request #18685 from carolahp/creanup/syc-new-method-extract-command2

ReExtractMethodDriver fixes from Balsa's review

Author: jecisc

src/Refactoring-Core-Tests/RBSubtreeDoesNotContainReturnConditionTest.class.st

@@ -21,6 +21,21 @@ RBSubtreeDoesNotContainReturnConditionTest >> testFailureWhenSubtreeContainsRetu
 	^ 'Just for test purpose' "maybe this is a bad thing to do in a test, I can change it if needed"
 ]
 
+{ #category : 'tests' }
+RBSubtreeDoesNotContainReturnConditionTest >> testFailureWhenSubtreeContainsReturnInBlockExpectFalse [
+
+	| precondition model class parseTree subtree x |
+	model := self modelOnClasses: { self class }.
+	class := model classObjectFor: self class.
+	parseTree := class parseTreeForSelector: self selector.
+	subtree := parseTree extractSubtreeWith: 'x:=[^1]'.
+	
+	precondition := ReSubtreeDoesNotContainReturnCondition new subtree: subtree.
+	
+	self deny: precondition check.
+	x:=[^1] "maybe this is a bad thing to do in a test, I can change it if needed"
+]
+
 { #category : 'tests' }
 RBSubtreeDoesNotContainReturnConditionTest >> testSubtreeDoesNotContainReturnExpectTrue [
 
@@ -34,3 +49,18 @@ RBSubtreeDoesNotContainReturnConditionTest >> testSubtreeDoesNotContainReturnExp
 
 	self assert: precondition check
 ]
+
+{ #category : 'tests' }
+RBSubtreeDoesNotContainReturnConditionTest >> testSubtreeIsAnAssignmentExpectTrue [
+
+	| precondition model class parseTree subtree x |
+	model := self modelOnClasses: { self class }.
+	class := model classObjectFor: self class.
+	parseTree := class parseTreeForSelector: self selector.
+	subtree := parseTree extractSubtreeWith: 'x := 42'.
+	
+	precondition := ReSubtreeDoesNotContainReturnCondition new subtree: subtree.
+	
+	self assert: precondition check.
+	x := 42 "maybe this is a bad thing to do in a test, I can change it if needed"
+]

src/Refactoring-Core/ReHasSameExitPointAsItsMethodCondition.class.st

@@ -19,5 +19,5 @@ ReHasSameExitPointAsItsMethodCondition >> check [
 { #category : 'displaying' }
 ReHasSameExitPointAsItsMethodCondition >> violationMessageOn: aStream [ 
 
-	^ aStream nextPutAll: 'Extracting guard clauses and other expressions that directly affect a method’s execution flow may result in unreachable code in the original method.'
+	^ aStream nextPutAll: 'Extracting guard clauses and other expressions that directly affect a method’s execution flow may cause unexpected behavior in the original method.'
 ]

src/Refactoring-UI/ReExtractAndBrowseMethodOverridesChoice.class.st

@@ -0,0 +1,20 @@
+Class {
+	#name : 'ReExtractAndBrowseMethodOverridesChoice',
+	#superclass : 'ReMethodChoice',
+	#category : 'Refactoring-UI-Choices',
+	#package : 'Refactoring-UI',
+	#tag : 'Choices'
+}
+
+{ #category : 'accessing' }
+ReExtractAndBrowseMethodOverridesChoice >> action [
+
+	driver extractMethod.
+	driver browseOverrides
+]
+
+{ #category : 'accessing' }
+ReExtractAndBrowseMethodOverridesChoice >> description [
+
+	^ description ifNil: [ description := 'Extract method and browse existing methods that will then be overriden' ]
+]

src/Refactoring-UI/ReExtractMethodChoice.class.st

@@ -0,0 +1,19 @@
+Class {
+	#name : 'ReExtractMethodChoice',
+	#superclass : 'ReMethodChoice',
+	#category : 'Refactoring-UI-Choices',
+	#package : 'Refactoring-UI',
+	#tag : 'Choices'
+}
+
+{ #category : 'accessing' }
+ReExtractMethodChoice >> action [
+
+	driver extractMethod
+]
+
+{ #category : 'accessing' }
+ReExtractMethodChoice >> description [
+
+	^ description ifNil: [ description := 'Extract method' ]
+]

src/Refactoring-UI/ReExtractMethodDriver.class.st

@@ -28,15 +28,13 @@ ReExtractMethodDriver >> breakingChoices [
 	items := OrderedCollection new.
 
 	(notOverride check and: sameExitPoint check) ifFalse: [
-			items add: (ReRenameMethodChoice new
+			items add: (ReExtractMethodChoice new
 					 driver: self;
-					 description: 'Extract method';
 					 yourself) ].
 
 	notOverride check ifFalse: [ "These choices are exclusive to notOverride"
-			items add: (ReRenameAndBrowseMethodOverridesChoice new
+			items add: (ReExtractAndBrowseMethodOverridesChoice new
 					 driver: self;
-					 description: 'Extract method and browse existing methods that will then be overriden';
 					 yourself).
 			items add: (ReBrowseMethodOverridesChoice new
 					 driver: self;
@@ -49,10 +47,9 @@ ReExtractMethodDriver >> breakingChoices [
 { #category : 'actions' }
 ReExtractMethodDriver >> browseOverrides [
 
-	| conditions overrides |
-	conditions := refactoring failedBreakingChangePreconditions.
-	overrides := conditions flatCollect: [ :cond |
-		             cond violators collect: [ :cls | cls realClass methodNamed: newMessage methodName asSymbol ] ].
+	| overrides |
+	overrides := notOverride violators collect: [ :cls | cls realClass methodNamed: newMessage methodName asSymbol ].
+	
 	overrides do: [ :method | method browse ]
 ]
 
@@ -118,6 +115,12 @@ ReExtractMethodDriver >> extract: aString from: compiledMethod in: aClass [
 	body := aString
 ]
 
+{ #category : 'actions' }
+ReExtractMethodDriver >> extractMethod [
+	
+	^ self applyChanges
+]
+
 { #category : 'execution' }
 ReExtractMethodDriver >> handleBreakingChanges [
 
@@ -149,14 +152,6 @@ ReExtractMethodDriver >> methodNameEditorPresenterClass: aClass [
 	methodNameEditorPresenterClass := aClass
 ]
 
-{ #category : 'actions' }
-ReExtractMethodDriver >> renameMethod [
-	"Eventhough we are not renaming methods, but extracting them, we implement this action to reuse both
-	ReRenameAndBrowseMethodOverridesChoice and ReRenameMethodChoice"
-
-	^ self applyChanges
-]
-
 { #category : 'accessing' }
 ReExtractMethodDriver >> requestDialogWith: methodName [
 	"This is lazy loaded and tests expect lazy loading, because they set `requestDialog`

src/Refactoring-UI/StMethodNameEditorPresenter.class.st

@@ -228,6 +228,12 @@ StMethodNameEditorPresenter >> canRenameArgs: anObject [
 	argumentsList items do: [ :arg | arg canBeRenamed: anObject ]
 ]
 
+{ #category : 'accessing' }
+StMethodNameEditorPresenter >> cancelled [
+	
+	^ self withWindowDo: #cancelled
+]
+
 { #category : 'action' }
 StMethodNameEditorPresenter >> computePermutation [