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

[RFC] Don't enforce `self` type in `equals` method #1

Closed
wants to merge 1 commit into from

Conversation

@hhamon
Copy link
Contributor

hhamon commented Mar 13, 2020

Thanks @lyrixx for the component,

I suggest not to enforce the typehint for the equals method to enable more flexibility.

This would allow one to do write code as is:

$uuid->equals($anObjectOfAnotherType); // false
$uuid->equals($anotherUuidObject); // true / false
$uuid->equals($aNullValue); // false
$uuid->equals('974d05ab-835e-4d58-af3d-88a0666c5e74'); // true / false

What do you think?

PS: I'll update unit tests if a consensus agrees on this change.

return 0 === uuid_compare($this->uuid, $other->uuid);
}

if (is_string($other)) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 13, 2020

Member

I think accepting strings makes the method less useful as it forces another check to know the type.

I'm good with accepting other types and returning false.

This comment has been minimized.

Copy link
@hhamon

hhamon Mar 13, 2020

Author Contributor

Sounds legit

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 13, 2020

Member

don't forget to add a test :)

@kunicmarko20

This comment has been minimized.

Copy link

kunicmarko20 commented Mar 13, 2020

Shouldn't this PR be on https://github.com/symfony/symfony?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 13, 2020

@kunicmarko20 totally!
@hhamon closing therefor.

@hhamon

This comment has been minimized.

Copy link
Contributor Author

hhamon commented Mar 13, 2020

Oups indeed, I'am opening it on Symfony repository

@hhamon

This comment has been minimized.

Copy link
Contributor Author

hhamon commented Mar 13, 2020

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.