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

Add index parameter to StatementList::addStatement #664

Merged
merged 1 commit into from
May 9, 2016

Conversation

thiemowmde
Copy link
Contributor

I'm proposing this as the most simple fix to get rid of StatementListHolder. This is currently not possible because there is no way to re-implement the index feature in ChangeOpStatement without having an Entity::setStatements or at least StatementList:clean method.

  • Currently, ChangeOpStatement turns the StatementList into an array, does all the index stuff, recreates a new StatementList and calls setStatements. Main issue: We decided we want to get rid of this setter together with all "holder" interfaces. This now turned into a pressing issue because we do not want MediaInfo to implement the deprecated holder interface.
  • ChangeOpStatement could, in theory, clean the StatementList (either with a clear method we do not want for good reasons, see Add clear to list classes #649, or by calling the existing removeStatementsWithGuid in a loop, which qualifies as a hack). The StatementList can then be re-filled by re-adding all statements in the new order. Again, this is more "a hack on top of a hack" than a clean solution.
  • With the proposed index parameter ChangeOpStatement becomes almost trivial.

You may wonder why this is the only StatementList method that exposes this index. Don't be fooled: toArray exposes the indexes. Whatever a user wants to know about an index (e.g. find statements by index or query for an index by GUID, main snak or property ID), I propose to not implement all this as StatementList methods. Instead, let the user iterate the array. The only guarantee toArray must give then (and already gives, see #466) is that the array keys correctly reflect the indexes.

This new index parameter fits, in my opinion, perfectly fine in the overall contract of the class: It's thin wrapper around a numerically index array of statements, not ordered or grouped in any way.

This is identical to ReferenceList::addReference.

Bug: T133853

@thiemowmde thiemowmde added this to the 6.1.0 milestone Apr 28, 2016
@JeroenDeDauw JeroenDeDauw changed the title Add index parameter to Statement::addStatement Add index parameter to StatementList::addStatement Apr 28, 2016
@JeroenDeDauw
Copy link
Contributor

Seems reasonable enough. Reminds me of how Claims got messed up, lets be careful to not repeat that

@thiemowmde
Copy link
Contributor Author

Totally agree. See #666 for a possible deprecation in this class.

@adrianheine
Copy link
Contributor

Ok. I think I would have preferred a new method addStatementAt with non-optional position argument, but this is ok.

@JonasKress JonasKress merged commit 403c110 into master May 9, 2016
@JonasKress JonasKress deleted the addStatementAtIndex branch May 9, 2016 10: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.

4 participants