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

Comprehensive entity editing #570

Merged
merged 101 commits into from Jul 28, 2021
Merged

Comprehensive entity editing #570

merged 101 commits into from Jul 28, 2021

Conversation

robertvazan
Copy link
Collaborator

This PR includes entity editing via WikibaseDataEditor.editEntityDocument. Most of the code consists of update representations and update builders. All edit operations (add/modify/remove all parts of all entity types) have been manually tested. Some related changes have been omitted though, because this is already a moster PR:

  • Lots of convenience methods on builders have been omitted.
  • Builder constructor fromUpdate(EntityUpdate) has been omitted.
  • Classes DatamodelConverter, DatamodelFilter, ToString, and ValueVisitor need new methods to support the new EntityUpdate interfaces.
  • Examples are still using the old deprecated APIs.
  • EntityUpdateImpl classes don't have validation. They rely on validation in EntityUpdateBuilder classes.
  • There are no unit tests. The code has been however tested manually using special test app on test Wikidata.
  • Special-casing of edit operations works only for APIs that were already implemented. WDTK currently does not have implementation for wbsetsitelink, wbsetclaimvalue, wbsetqualifier, wbsetreference, wbremovequalifiers, wbremovereferences.

Some of this missing functionality needs further discussion. I think all of it needs separate PR in order to limit size of this PR. Some of these should be a separate issue.

This PR resolves #437, #376, and most of #403.

@robertvazan
Copy link
Collaborator Author

Just FYI, I have added validation and tests for XUpdateImpl classes. Only JSON serialization and request optimization are left to test.

@robertvazan
Copy link
Collaborator Author

Codecov is a bit flaky, but I think I have decent code coverage by now. I have settled on testing WikibaseDataEditor by mocking underlying WbEditingAction. This resulted in fairly clean tests that do not try to test too much. JSON serialization tests are in XImplTest classes.

Remaining missing changes:

  • Lots of convenience methods on builders have been omitted.
  • Builder constructor fromUpdate(EntityUpdate) has been omitted.
  • Classes DatamodelConverter, DatamodelFilter, ToString, and ValueVisitor need new methods to support the new EntityUpdate interfaces.
  • Examples are still using the old deprecated APIs.
  • Special-casing of edit operations works only for APIs that were already implemented. WDTK currently does not have implementation for wbsetsitelink, wbsetclaimvalue, wbsetqualifier, wbsetreference, wbremovequalifiers, wbremovereferences.

Builders follow a naming convention that makes it easy to add convenience methods:

  • updateX: apply YUpdate
  • addX: add item
  • removeX: remove item
  • replaceX: replace existing item (fail if it does not exist)
  • putX: add or replace an item
  • setX: set singleton field
  • X.append(): merge existing XUpdate on top of current builder state

So for example ItemUpdateBuilder can have convenience method putLabel(x), which would be equivalent to current updateLabels(TermUpdate.create().put(x).build()).

Please review.

@robertvazan robertvazan requested review from Tpt and wetneb July 23, 2021 23:02
Copy link
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

Thank you so much! The tests address my concerns entirely.

I haven't had a close look at the new editing API but it's easier if I simply do so while updating OpenRefine to the new API. I can imagine I might have suggestions of things to change then but it will be much easier to address them individually rather than by requesting changes to this big PR :) All in all I think this is definitely going in the right direction and seems to have been done with a lot of care, so again thank you!

Copy link
Collaborator

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

I have not reviewed the code carefully but it seems good to me. As @wetneb said we can definitely fix things afterward. Feel free to merge whenever you think it's appropriate.

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

Successfully merging this pull request may close these issues.

Add Lexeme editing
3 participants