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

Use differ class name in differStrategies array as key #138

Closed
wants to merge 1 commit into from

Conversation

JonasKress
Copy link
Contributor

@JonasKress JonasKress commented May 3, 2016

@adrianheine
Copy link
Contributor

Mh, this would currently work, but there might be different instances of the same class that diff different things.

@thiemowmde
Copy link
Contributor

thiemowmde commented May 3, 2016

  • Misses tests.
  • Is this relevant for a Phabricator ticket?
  • What Adrian said. The class name is not a good identifier.
  • I wonder why this is needed? Sure, to avoid duplicates in the array. But even if there are duplicates, when and how is this a problem? The loop stops the moment it finds the first differ that can diff the given entity type.
  • The only effect of this patch is that the differ added last will be picked, instead of the first.
  • This makes this a breaking change.
  • See Add ItemAndPropertyDiffer #61.

@JonasKress
Copy link
Contributor Author

JonasKress commented May 3, 2016

@thiemowmde
Copy link
Contributor

In my opinion, a proper fix would be to remove the default constructor. Or even better, deprecate this class and introduce a DispatchingEntityDiffer, either here or in repo where it is needed.

In any case, not critical because the duplicates don't do anything.

@JeroenDeDauw
Copy link
Contributor

There is no need to make any breaking changes to this class. I've looked at the commit on Gerrit and can see how what this class provides is not ideal for that user. What I suggest is done is to extract the relevant code from this class into a new one that requires the strategies in its constructor and has no default ones. Existing users remain well served and unaffected, and the Gerrit one can use the new class.

@JeroenDeDauw
Copy link
Contributor

Presumably no longer relevant. Please reopen if wrong

@JeroenDeDauw JeroenDeDauw deleted the uniqueDifferStrategy branch August 8, 2018 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants