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.

Sign up for GitHub

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

Jump to bottom

[Addressing] Simplify AddressComparator #6541

Merged
merged 1 commit into from
Oct 26, 2016
Merged

[Addressing] Simplify AddressComparator #6541

merged 1 commit into from
Oct 26, 2016

Conversation

pamil
Copy link
Contributor

@pamil pamil commented Oct 25, 2016

Q A
Bug fix? yes (static calls to non-static method)
New feature? no
BC breaks? no
Related tickets related to #6536
License MIT

@pamil pamil mentioned this pull request Oct 25, 2016
Merged
@pjedrzejewski pjedrzejewski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Oct 25, 2016
@liverbool
Copy link
Contributor

liverbool commented Oct 25, 2016 •
edited
Loading

👍 But my concern, When someone who extends Address he must to decorate this service too, Can we have magic and safe logic?

I thinks about using ReflectionClass::getMethods since we need to compare all method except id, right?

@pamil
Copy link
Contributor Author

pamil commented Oct 25, 2016 •
edited
Loading

@liverbool that's right, if someone adds a method to AddressInterface then this service needs customisation too.

We should not compare neither all methods nor all properties of the object being passed as:

  • it may be an instance of a different class than the other object
  • it may have additional methods, that are not defined on interface, but do not change comparison logic
  • it may have additional properties, that do not change comparison logic

This is rather not a magic and safe logic 🎉

@pamil pamil force-pushed the simplify-address-comparator branch from 2620424 to 8741735 Compare October 26, 2016 08:11
@pamil pamil force-pushed the simplify-address-comparator branch from 8741735 to 23b0a01 Compare October 26, 2016 08:30
@pjedrzejewski pjedrzejewski merged commit 4e16848 into Sylius:master Oct 26, 2016
@pjedrzejewski
Copy link
Member

Thank you Kamil!

@pamil pamil deleted the simplify-address-comparator branch October 26, 2016 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pamil @liverbool @pjedrzejewski