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: deprecate class driver #15560

Merged

Conversation

balsa-sarenac
Copy link
Member

@balsa-sarenac balsa-sarenac commented Nov 25, 2023

This PR introduces interactive driver for deprecate class family of transformations.
Driver offers user three options:

  • Just add isDeprecated method to the class side (@MarcusDenker I think you'll like this one :))
  • Rename class, add a subclass with old class name and deprecate it
  • Deprecate class and migrate references to another class

I tried to clean as much as possible, but there is still some work left. I will get back to this after I clean the API a bit.

New driver can be initalized through:
Right click on a class -> Refactoring -> Deprecate

Right now we have duplicate commands that don't use driver. Those are:
Right click on a class -> Deprecate (this subclasses and renames the class)
Right click on a class -> Migrate references (deprecate and migrate references)
If the driver commands seem good, we can remove these commands.

…e class side

Previously, this transformation overrided `new` and `systemIcon` methods that
are not needed to override.
Also, it was incorrect since it was creating `deprecated` method which doesn't
not make the class deprecated.
Finally, we've removed comment creation since new comment overrides exisitng
one, and it's currently very hard to preserve the old comment and prepend it with
"Deprecated". But the system strikes through the class, so we have visual
representation, which should be enough.
Selectors were misleading because instead of `class` they expected `className`
We've figured a simpler order of transformations:
1. Rename old class name to new class name
2. Add new subclass with the name of old class
3. Deprecate newly created subclass
User can choose between:
- Rename old class, migrate the references to new class and deprecate the old class
- Migrate references to an existing class and deprecate the class
`applicabilityPreconditions` and `checkApplicabilityPreconditions`
are used both by refactorings and transformations so it makes
sense to have them in the common superclass.
@jecisc
Copy link
Member

jecisc commented Nov 25, 2023

Hi!
Since Pharo 12 it is better to not add deprecated subclasses because they have multiple problems:

  • #isKindOf:/on:do: and related methods might not always work the expected ways
  • if the class has a special registration mechanism it might not work
  • Extension methods will be placed on the subclass and the renamed class will not know them

Instead we can add a sentence to the class initialization you do self deprecatedAliases: #( #DeprecatedClassName )

in the future I want to add this to the class definition instead of going through the initialize

@balsa-sarenac
Copy link
Member Author

balsa-sarenac commented Nov 25, 2023

Hey @jecisc,

I now remember that we talked about this back in September. I will look at how #deprecateAliases: is used in the image and see if I can easily add it. Thanks for the heads up!

@Ducasse
Copy link
Member

Ducasse commented Nov 26, 2023

I would be good to enter an issue to remove a refactoring that does not make sense because it is less work for us.
And now Balsa worked for nothing.

@Ducasse
Copy link
Member

Ducasse commented Nov 26, 2023

So what do we do.
I'm for removing everything related to class deprecation and if people wants one then they will write one.
And they will have to follow our architecture.

@balsa-sarenac
Copy link
Member Author

To use deprecatedAliases: one needs to add a line to the initalize with name of deprecated aliases. I guess this one should be used both for rename and deprecate and for migrate references.
I can work with this and make RBDeprecateTransformation can add deprecatedAliases: to initialize.
Both Rename and Migrate can then be modified to use RBDeprecateTransformation to create deprecatedAliases: and their own logic?

What do you think of this?

@Ducasse
Copy link
Member

Ducasse commented Nov 26, 2023

I can integrate it now and we can add it or not.
Else we remove them

@Ducasse Ducasse merged commit 89dd9d9 into pharo-project:Pharo12 Nov 26, 2023
3 checks passed
@balsa-sarenac balsa-sarenac deleted the feat/deprecate-class-driver 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
Status: DONE
Development

Successfully merging this pull request may close these issues.

None yet

3 participants