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

Make class abstract driver #16347

Merged
merged 10 commits into from
May 21, 2024

Conversation

hernanmd
Copy link
Contributor

This PR implements the "Make Abstract" class refactoring using the new Driver interface.

Make RBAbstractClass match the MOP answering both false by default in #isAbstract.
Migrate RBMakeClassAbstractTransformation.
Add data for testing: #RBClassWithoutSelfClassReferenceTest and #RBWithSelfClassReferenceTest
Update RBMakeClassAbstractParametrizedTest
Add #RBMakeClassAbstractDriver with tests
Update #SycCMakeAbstractCommand

@Ducasse
Copy link
Member

Ducasse commented Mar 25, 2024

What is the logic of this refactoring? I mean I do not really understand what is an abstract class in pharo and when i will need it.

Hernán Morales Durand and others added 2 commits March 25, 2024 21:51
… #isAbstract.

Migrate RBMakeClassAbstractTransformation.
Add data for testing: #RBClassWithoutSelfClassReferenceTest and #RBWithSelfClassReferenceTest
Update RBMakeClassAbstractParametrizedTest
Add #RBMakeClassAbstractDriver with tests
Update #SycCMakeAbstractCommand
@hernanmd
Copy link
Contributor Author

I migrated because it was there and some people may be using it.

However, I never bought the arguments for "improving code clarity" or "preventing improper instantiation" (maybe they're good for educational purposes), so I'll provide an example where I used it some time ago as a dirty experiment: To migrate a legacy system, using automated scripts, some classes couldn't be migrated to the new system. To get a quick diff of which ones weren't migrated, I annotated the already migrated abstract classes with the "isAbstract" method, so I could later see which hierarchies have fewer subclasses than expected.

If there are other use cases I would have to think about them because as you said, there doesn't seem to be too many.

@MarcusDenker
Copy link
Member

I use it a lot, it is useful for the Browser and the Rules to know that a class is abstract. Imagine having a subclassResponsibility method. The browser nags you to implement it, but the subclass is Abtract, too. So we could add the method again, then it complaints that the method is the same as in the superclass.

=> add #isAbstract, and the Browser/Rules know that you do not have to implement the #subclassResponsability here and just ask you to implement it where you want it

@jecisc
Copy link
Member

jecisc commented Mar 26, 2024

I also like to know easily when a class is abstract when I'm exploring a project I'm not familiar with. Like that I'm able to understand the structure of the project more easily.

And I know that I should not instantiate those classes but just look at the subclasses.

hernanmd and others added 4 commits March 28, 2024 14:04
@jecisc
Copy link
Member

jecisc commented Mar 28, 2024

CI got a random failure. I'm restarting it

@hernanmd
Copy link
Contributor Author

Thanks Cyril, it seems that failing tests are not related:

Screenshot 2024-03-28 at 16 37 42

@jecisc
Copy link
Member

jecisc commented Mar 28, 2024

Some tests are refactoring related so it's possible it comes from here no?

@hernanmd
Copy link
Contributor Author

hernanmd commented Mar 29, 2024

Some tests are refactoring related so it's possible it comes from here no?

Ok, I fixed the failing tests. Now it seems good to go

https://ci.inria.fr/pharo-ci-jenkins2/blue/organizations/jenkins/Test%20pending%20pull%20request%20and%20branch%20Pipeline/detail/PR-16347/6/tests

@estebanlm estebanlm changed the base branch from Pharo12 to Pharo13 April 26, 2024 09:11
@jecisc jecisc closed this May 21, 2024
@jecisc jecisc reopened this May 21, 2024
@jecisc jecisc merged commit 0df3330 into pharo-project:Pharo13 May 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants