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

Streamline FingerprintPatcher #120

Merged
merged 1 commit into from Apr 20, 2016
Merged

Streamline FingerprintPatcher #120

merged 1 commit into from Apr 20, 2016

Conversation

thiemowmde
Copy link
Contributor

This streamlines the implementation and avoids calling the methods Fingerprint::setLabels, setDescriptions and setAliasGroups.

Missing:

  • Refactor alias patching the same way. Done.
  • Add tests. Lots and lots of them. All the conflicts are currently not tested! Done.
  • Split code. Bad nesting. Done.

Bug: T78298

@thiemowmde thiemowmde force-pushed the fpPatcher branch 2 times, most recently from a864b9c to 7ce5b8a Compare March 9, 2016 15:26
@Benestar Benestar added this to the 3.5 milestone Mar 10, 2016
@thiemowmde thiemowmde force-pushed the fpPatcher branch 3 times, most recently from 9ff6c23 to 67ecb24 Compare March 11, 2016 11:51
@thiemowmde thiemowmde changed the title [WIP] Streamline FingerprintPatcher Streamline FingerprintPatcher Mar 11, 2016
@thiemowmde
Copy link
Contributor Author

I did an additional benchmark. Memory usage is unaffected. Runtime drops from 500ms to 200ms in my worst case test.

@brightbyte
Copy link

Removing milestone after discussion with Thiemo. This is a non-critical refactoring.

@JeroenDeDauw
Copy link
Contributor

It's somewhat unfortunate to have to write custom patching code like this. I can see the benefits, though to me it's unclear if those outweigh the drawbacks. Don't let that stop you though, if you think this is better and worth the time, then go for it.

@thiemowmde
Copy link
Contributor Author

Rebased on top of #124.

* @throws PatcherException
* @return string[]
*/
private function getPatchedAliases( array $aliases, Diff $patch ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This whole method could be replaced with a ListPatcher call, but this needs wmde/Diff#65.

@adrianheine adrianheine merged commit f337392 into master Apr 20, 2016
@adrianheine adrianheine deleted the fpPatcher branch April 20, 2016 08:09
@thiemowmde thiemowmde modified the milestone: 3.6.0 May 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants