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

Feat: Add Override method transformation #15225

Merged

Conversation

balsa-sarenac
Copy link
Member

This is a proposal for new refactoring that will be useful in composition.

Regular AddMethodRefactoring does not allow override of a method from superclass or the class.
In composition it's usually only important not to override existing method from the class, and most of the refactorings additionally check this and use AddMethodTransformation, because AddMethodRefactoring breaking change is too restrictive.

This new refactoring allows override from methods from superclass, but it does not allow override of method directly defined in the class.

It is required in multiple places, but I cannot fully refactor it now. I will get back to this when we start working on composition again and have clear solution for checking preconditions. I just put placeholders for the future.

In some places it's okay to override method from superclass,
but not override method from class. This should make it easy
to spot the difference in semantics.
@Ducasse
Copy link
Member

Ducasse commented Nov 4, 2023

Ok for me. This is a subtle difference. I would like to know why this is not a transformation since it is changing behavior. But I integrate it now.

@balsa-sarenac
Copy link
Member Author

balsa-sarenac commented Nov 4, 2023

Good point, it's a transformation definitely, I will fix it in this PR since there are some failing tests I need to fix as well.

@Ducasse
Copy link
Member

Ducasse commented Nov 4, 2023

Balsa

osx-64 / Tests-osx-64 / testExistingClassVariableAccessors(#rbClass->RBAddVariableAccessorWithLazyInitializationTransformation) – MacOSX64.Refactoring.Transformations.Tests.Parametrized.RBAddVariableAccessorsWithLazyInitializationParametrizedTest
<1s
osx-64 / Tests-osx-64 / testExistingInstanceVariableAccessors(#rbClass->RBAddVariableAccessorWithLazyInitializationTransformation) – MacOSX64.Refactoring.Transformations.Tests.Parametrized.RBAddVariableAccessorsWithLazyInitializationParametrizedTest
<1s
osx-64 / Tests-osx-64 / testDeprecateMethodUsingMethodWithSameNumberOfArgs(#rbClass->RBDeprecateMethodTransformation) – MacOSX64.Refactoring.Transformations.Tests.Parametrized.RBDeprecateMethodParametrizedTest
6s
osx-64 / Tests-osx-64 / testDeprecateMethodUsingMethodWithoutArgs(#rbClass->RBDeprecateMethodTransformation) – MacOSX64.Refactoring.Transformations.Tests.Parametrized.RBDeprecateMethodParametrizedTest

@balsa-sarenac
Copy link
Member Author

@Ducasse I've reverted references and will fix that in the future. These refactorings need to raise warnings, and for that they need driver. So this PR introduces new transformation, in a later one there will be a driver for it.

@balsa-sarenac balsa-sarenac changed the title Feat: Add Override method Refactoring Feat: Add Override method transformation Nov 4, 2023
@Ducasse
Copy link
Member

Ducasse commented Nov 4, 2023

Ok Thursday we should have a meeting with caro and I think that she should focus on the drivers.
Think about a better list than the last time.

@Ducasse
Copy link
Member

Ducasse commented Nov 4, 2023

I will merge when build is ready.

@Ducasse Ducasse merged commit e42eeea into pharo-project:Pharo12 Nov 4, 2023
1 of 2 checks passed
@balsa-sarenac balsa-sarenac deleted the feat/add-override-method branch January 26, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants