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

Clean up entity patching code #196

Merged
merged 15 commits into from Sep 22, 2014
Merged

Clean up entity patching code #196

merged 15 commits into from Sep 22, 2014

Conversation

JeroenDeDauw
Copy link
Contributor

TL;DR: split patching code from Entity and did the same with its tests

Done:

  • split patching code from Entity and derivatives
  • drop usage of deprecated term related methods defined by Entity
  • split Fingerprint patching code into its own class
  • split Statement patching code into its own class
  • split SiteLink patching code into its own class
  • drop usage of deprecated claim related methods defined by Entity
  • drop usage of deprecated Item methods
  • fully replicate the old tests and then remove them from EntityTest and derivatives

@JeroenDeDauw
Copy link
Contributor Author

@WMDE-Fisch @Benestar FYI

@JeroenDeDauw JeroenDeDauw added this to the 1.1 milestone Sep 18, 2014
* @throws InvalidArgumentException
* @throws RuntimeException
*/
public function patchEntity( EntityDocument $entity, EntityDiff $patch ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wtf?? Guess I need to file a fifth PhpStorm 8.0 bug then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or not. Added in 186fbd7, so guess @WMDE-Fisch does not have his IDE set to tabs. Heresy! :)

@JeroenDeDauw JeroenDeDauw changed the title [WIP] Clean up entity patching code Clean up entity patching code Sep 18, 2014
@JeroenDeDauw
Copy link
Contributor Author

Let's do some bets :)

ratingz

  • EntityTest F -> E
  • ItemTest D -> C
  • Entity B -> A
  • Item B -> A
  • Global score 9.06 -> 9.11

Edit: found you can cheat by checking https://scrutinizer-ci.com/g/wmde/WikibaseDataModel/inspections/3c5ffa61-9677-4995-8a9d-7bc119f0e878

  • EntityTest F -> C (yay)
  • ItemTest D -> D (sad)
  • Entity B -> B (sad)
  • Item B -> B (sad)
  • Global score 9.06 -> 9.23 (woot)

@JeroenDeDauw
Copy link
Contributor Author

@addshore For great rating! :)

$patchedEntity = $item->copy();
$patcher->patchEntity( $patchedEntity, new ItemDiff() );

$this->assertEquals( $item, $patchedEntity );
Copy link
Contributor

Choose a reason for hiding this comment

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

Still indentation issues


$item->setStatements( $this->statementListPatcher->getPatchedStatementList(
$item->getStatements(),
$patch->getClaimsDiff()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a problem of this patch but we have to remember that this isn't very sane. EntityDiff should not know about claims.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should also not know about fingerprints. And the "claims" in the names here should be changed to "statements". That's an EntityDiff problem though, which is for another time.

@Benestar
Copy link
Contributor

-1 for the Statement/Claim confusion

@JeroenDeDauw
Copy link
Contributor Author

-1 for the Statement/Claim confusion

Removed now

* @licence GNU GPL v2+
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
interface EntityPatcherStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the benefit of this interface. EntityDocument is only about ids and the ids will never get patched because they are immutable. Therefore we will also most likely kill the EntityDiff class entirely because it contains now EntityStuff but only stuff specific for fingerprints or claims/statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we have this interface is so that we can have EntityPatcher, or rather the functionality EntityPatcher provides. The users of DataModel have places where they have something of type Entity and need to patch it. In many cases having something of an unknown Entity types is bad, but still DM needs to support its current users. This dispatcher + strategy approach allows DM to support this without itself having any patching logic that deals with entities of unknown type.

tobijat added a commit that referenced this pull request Sep 22, 2014
Clean up entity patching code
@tobijat tobijat merged commit c0403ba into master Sep 22, 2014
@tobijat tobijat deleted the entitypatcher branch September 22, 2014 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants