Skip to content
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.

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

Removing classes should not warn about references in classes to remove #4294

Closed
Lin777 opened this issue Aug 12, 2019 · 4 comments

Comments

@Lin777
Copy link
Contributor

commented Aug 12, 2019

If I want to remove a class A and a class B with B referencing A, currently, Calypso will warn me that A is still referenced before applying the refactoring.

IMO this should not happen because the class referencing A is part of the classes to remove.

Lin777/Enjoliveur#41

@bencoman

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

What are the steps to reproduce?
Please provide a script that can be run in a freshly downloaded image to:

  1. create a condition where class B is referencing class A,
  2. continue on produce the warning by trying to remove class A .
@Lin777

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Steps for references warning:

  1. Create the classes: A, B where A refers to B.
  2. Select classes A, B and delete them (Pressing Ctrl-X)
  3. Produces a warning: There are 1 references to B, A. Show them?

Steps for subclass warning:

  1. Create the classes: A, B where B is subclass of A
  2. Select A, B and delete them.
  3. Produces a warning: There are subclasses. Show them?

This should not happen since references and subclasses are also being deleted.

@bencoman

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Thanks Evelyn. So to confirm, the code below is equivalent?...

Object subclass: #MyA
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'Issue4294'.
	
Object subclass: #MyB
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'Issue4294'.

#MyA asClass compile: 'reference
    ^ MyB'.

cmd := SycRemoveClassCommand new classes: {#MyA asClass. #MyB asClass}.
cmd execute
@Lin777

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Yes, it is equivalent and to test the subclasses error the script would be like this:

Object subclass: #MyA
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'Issue4294'.
	
MyA subclass: #MyB
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'Issue4294'.

cmd := SycRemoveClassCommand new classes: {#MyA asClass. #MyB asClass}.
cmd execute.

This will generate errors of the type RBRefactoringError, since it gets to execute the method primitiveExecute.

However, when executing from the UI, first execute this: aToolContext confirmUnusedClasses: classes from the method: #prepareFullExecutionInContext: which throws errors as alerts.

Therefore it performs a double validation and both already fix them in this PR: #4295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.