Skip to content

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.

5 participants