Skip to content

[Refactoring] Fix generating accessors variable slots bug #18777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: Pharo14
Choose a base branch
from
+56 −14
Open

[Refactoring] Fix generating accessors variable slots bug #18777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e7082d
[Refactoring] Fix generating accessors variable slots bug
AlexisCnockaert Oct 28, 2025
960ee55
Merge branch 'Pharo14' into refactoring/generateaccessors
AlexisCnockaert Oct 28, 2025
bc96dcd
clean the way it is handled looking at context side when calling the …
AlexisCnockaert Oct 28, 2025
7546875
Merge 960ee55677b7aa3936791786b2e5fea774c1e2c6
AlexisCnockaert Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Calypso-SystemQueries/ClyClassSideScope.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,22 @@ ClyClassSideScope class >> defaultName [
^'class side'
]

{ #category : 'testing' }
ClyClassSideScope class >> isInstanceSide [

^ false
]

{ #category : 'class selection' }
ClyClassSideScope class >> metaLevelOf: aClass [
^aClass classSide
]

{ #category : 'testing' }
ClyClassSideScope >> isInstanceSide [
^ self class isInstanceSide
]

{ #category : 'queries' }
ClyClassSideScope >> methodsDo: aBlock [

Expand Down
11 changes: 11 additions & 0 deletions src/Calypso-SystemQueries/ClyInstanceSideScope.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,18 @@ ClyInstanceSideScope class >> defaultName [
^'inst. side'
]

{ #category : 'testing' }
ClyInstanceSideScope class >> isInstanceSide [

^ true
]

{ #category : 'class selection' }
ClyInstanceSideScope class >> metaLevelOf: aClass [
^aClass instanceSide
]

{ #category : 'testing' }
ClyInstanceSideScope >> isInstanceSide [
^ self class isInstanceSide
]
6 changes: 6 additions & 0 deletions src/Calypso-SystemTools-Core/ClyClassDefinitionContext.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,9 @@ Class {
#package : 'Calypso-SystemTools-Core',
#tag : 'Editors-Classes'
}

{ #category : 'testing' }
ClyClassDefinitionContext >> isInstanceSide [
Copy link
Member

@Ducasse Ducasse Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this one strange.
And I also wondered why we could not use metaLevelOf: even if I do not like the name.

Copy link
Collaborator Author

@AlexisCnockaert AlexisCnockaert Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metaLevelOf: is for classes I think and there we want to check it for variables


^ selectedSourceNode parent class = CDClassDefinitionNode
]
29 changes: 21 additions & 8 deletions src/Refactoring-UI/ReGenerateAccessorsDriver.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ It uses the `RBGenerateEqualHashTransformation`.
Class {
#name : 'ReGenerateAccessorsDriver',
#superclass : 'ReGenerateMethodDriver',
#instVars : [
'scopeInstanceSide'
],
#category : 'Refactoring-UI-Drivers',
#package : 'Refactoring-UI',
#tag : 'Drivers'
Expand All @@ -20,15 +23,19 @@ ReGenerateAccessorsDriver class >> refactoringClass [

{ #category : 'configuration' }
ReGenerateAccessorsDriver >> configureRefactoring [

refactoring := ReUpFrontPreconditionCheckingCompositeRefactoring new
model: model;
refactorings: (selectedVariables collect: [:each |
self refactoringClass
model: model
instanceVariable: each
class: self targetClass name ]);
yourself.
model: model;
refactorings: (selectedVariables collect: [ :eachSlot | self refactoringForSlot: eachSlot ]);
yourself
]

{ #category : 'initialization' }
ReGenerateAccessorsDriver >> refactoringForSlot: aSlot [

^ scopeInstanceSide
ifFalse: [ self refactoringClass model: model classVariable: aSlot class: self targetClass name ]
ifTrue: [ self refactoringClass model: model instanceVariable: aSlot class: self targetClass name ]
]

{ #category : 'configuration' }
Expand All @@ -38,6 +45,12 @@ ReGenerateAccessorsDriver >> runRefactoring [
self applyChanges
]

{ #category : 'accessing' }
ReGenerateAccessorsDriver >> scopeInstanceSide: aBoolean [

scopeInstanceSide := aBoolean
]

{ #category : 'configuration' }
ReGenerateAccessorsDriver >> selectedVariables: aCollection [

Expand Down
13 changes: 7 additions & 6 deletions src/SystemCommands-VariableCommands/SycGenerateVariableAccessorCommand.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@ SycGenerateVariableAccessorCommand >> defaultMenuItemName [

{ #category : 'execution' }
SycGenerateVariableAccessorCommand >> execute [

(ReGenerateAccessorsDriver new
targetClass: variables first definingClass;
scopes: toolContext refactoringScopes;
selectedVariables: (variables collect: [:each | each name ]) ) runRefactoring


(ReGenerateAccessorsDriver new
targetClass: variables first definingClass;
Copy link
Contributor

@dionisiydk dionisiydk Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #definingClass actually gives you the correct class side object for class side variables.
You can detect the class side by:

variables first definingClass isInstanceSide

No need to analyze the toolContext at all. Variables are first class objects with the full information.

Copy link
Contributor

@dionisiydk dionisiydk Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is shame that new driver system is based on raw string names for variables

Copy link
Member

@jecisc jecisc Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to make it easy to use for scripting.
When you call it in a playground it's not nice to have to get variable objects.
But maybe we can add some API to configure the driver with real variables and then the driver gets the name and side of the variable for the refactoring?

Copy link
Member

@Ducasse Ducasse Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dionisiydk you are right. We should get two APIs.
I'm thinking that the driver in fact could work on variables (normally the drivers are not used in scripting mode).
So I will open an issues

scopes: toolContext refactoringScopes;
scopeInstanceSide: ((toolContext respondsTo: #metaLevelScope)
ifTrue: [ toolContext metaLevelScope isInstanceSide ]
ifFalse: [ toolContext isInstanceSide ]);
selectedVariables: (variables collect: [ :each | each name ])) runRefactoring
]

{ #category : 'factory method' }
Expand Down