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

Fixed the duplicate class refactoring wrong message #17941

Open
wants to merge 3 commits into
base: Pharo13
Choose a base branch
from

Conversation

AlexisCnockaert
Copy link
Collaborator

Fixes #17518

  • Indeed there is a <1?:not > in #isGlobal: in: that was putting a "not" in the linked issue but because we evaluated (RBCondition alreadyExistGlobal: className in: self model) not. the not was put in the wrong cases
  • I created a new method to handle it to not change anything in #isGlobal: in: to not break something

Fixes pharo-project#17518
- Indeed there is a  <1?:not > in #isGlobal: in: that was putting a "not" in the linked issue but because we evaluated 	```(RBCondition alreadyExistGlobal: className in: self model) not.``` the not was put in the wrong cases
- I created a new method to handle it to not change anything in #isGlobal: in: to not break something
@balsa-sarenac
Copy link
Member

I think you should just update the error message in the isGlobal: in: .
In my opinion this is just code duplication in order to have different error message.

Copy link
Member

@balsa-sarenac balsa-sarenac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check other methods how they introduced support for not. Make the error message you created the default in the old method. Make sure it works with not as well.

@AlexisCnockaert
Copy link
Collaborator Author

Ok got it thanks :)

@Ducasse
Copy link
Member

Ducasse commented Mar 14, 2025

I was thinking that we can integrate that now and we should convert everything to our condition and they should offer the possibility to change the message. I started to do that.

@balsa-sarenac
Copy link
Member

I agree, but this is easy to fix, I would suggest fixing now, it will be easier when transitioning later

@Ducasse
Copy link
Member

Ducasse commented Mar 14, 2025

Ok let us fix it now :)
You convinced me.

Copy link
Member

@balsa-sarenac balsa-sarenac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move on with this text, but this error message doesn't support not message, so we're creating a new issue.
I have to load the code to give some better suggestion on how to fix this. I'll try to find some time during next week.

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.

[UX] [RB] Duplicate class
3 participants