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

Migrate Refactoring for Generating Equal and Hash Methods #16786

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

hernanmd
Copy link
Member

Description

This PR migrates the refactoring browser for generating #= and #hash methods to the new driver interface. The new interface provides a more streamlined and extensible approach for generating these methods, making it easier to maintain and enhance the functionality in the future.

Motivation

The existing implementation of the refactoring browser for generating #= and #hash methods was tightly coupled with the user interface (UI) code and did not prevent overwriting the existing implementation of such methods (#16361). By separating the core logic from the UI, we can improve the code's maintainability and testability and enable future enhancements or alternative user interfaces.

Implementation.

The implementation of this PR involves the following steps:

  • Create a new driver interface: A new driver interface, ReGenerateEqualAndHashDriver, has been introduced. This interface defines the contract for generating #= and #hash methods, encapsulating the core logic for this functionality.
  • Implement the driver: The driver uses RBGenerateEqualHashTransformation for the underlying logic.
  • UI interaction: The driver uses SpSelectMultipleDialog for selecting the variables used in the generated #= and #hash methods.
  • Created tests: Tests related to the generation of #= and #hash methods have been provided in ReGenerateEqualAndHashDriverTest

Hernán Morales Durand and others added 3 commits June 17, 2024 00:03
Add SycCmGenerateEqualAndHashCommand (based in Commander 2)
Remove old SycGenerateEqualAndHashCommand using Commander 1.
Add icon
Add test class
It presents a selection dialog to confirm changes.
Add tests
@Ducasse
Copy link
Member

Ducasse commented Jun 19, 2024

@balsa-sarenac can you have a look? Tx

@Ducasse
Copy link
Member

Ducasse commented Jun 19, 2024

Yes

@Ducasse
Copy link
Member

Ducasse commented Jun 19, 2024

Sounds good to me.
Potentially the other menuString should be revisited.
But I will integrate the PR right now.

@hernanmd
Copy link
Member Author

Sounds good to me. Potentially the other menuString should be revisited. But I will integrate the PR right now.

Thanks

@Ducasse Ducasse merged commit 336ce79 into pharo-project:Pharo13 Jun 19, 2024
1 of 2 checks passed
@demarey
Copy link
Contributor

demarey commented Jun 20, 2024

@hernanmd The renaming is not good: Reprefix si used by Renraku classes (critics).
Also, we now have some release tests failing :

the following classes have unused instance variables and should be cleaned:ReClassForGeneratingEqualAndHashExistingImplementors ReClassForGeneratingEqualAndHash

ReleaseTest.testThatThereAreNoSelectorsRemainingThatAreSentButNotImplemented
 'ReClassForGeneratingEqualAndHashExistingImplementors>>#=

@hernanmd
Copy link
Member Author

ReClassForGeneratingEqualAndHashExistingImplementors

Thanks for reporting, Christophe.
Would you like me to fix it in another PR?

We should probably discuss the Re prefix (as taken by Renraku) with @Ducasse.

@hernanmd
Copy link
Member Author

hernanmd commented Jun 20, 2024

ReClassForGeneratingEqualAndHashExistingImplementors

Ok, I checked. Those instance variables are on purpose, so I added a Manifest.
(I cannot re-open the PR)

@demarey
Copy link
Contributor

demarey commented Jun 21, 2024

ReClassForGeneratingEqualAndHashExistingImplementors

Ok, I checked. Those instance variables are on purpose, so I added a Manifest. (I cannot re-open the PR)

Thank you for checking Hernan!

@demarey
Copy link
Contributor

demarey commented Jun 21, 2024

ReClassForGeneratingEqualAndHashExistingImplementors

Thanks for reporting, Christophe. Would you like me to fix it in another PR?

We should probably discuss the Re prefix (as taken by Renraku) with @Ducasse.

yes, I think it would be better to use another prefix. When you agree on a prefix, you can issue a new PR for it.

@Ducasse
Copy link
Member

Ducasse commented Jun 21, 2024

Why this is a problem to have two projects with the same prefix.
We could have ren for renraku and re for refactoring engine

@demarey
Copy link
Contributor

demarey commented Jun 21, 2024

Having the same prefix for different projects just add confusion even if technically, it does not change anything.
People are used to identify the project of a class by checking the prefix

@MarcusDenker
Copy link
Member

This PR was merged but it had failing tests, among them:

@MarcusDenker
Copy link
Member

I sadly can not even revert the PR as this is now so long ago that it was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants