Remove ByPropertyIdArray

[Benestar]

This class is full of issues, inefficient methods and code smells.

As a replacement, ByPropertyIdGrouper and ByPropertyIdMap (#479) can be used.

Tracked in https://phabricator.wikimedia.org/T98375

[JeroenDeDauw]

-2 on removing this before a viable alternative is merged. (done)

That being said, it'd be very happy to see this class go, so thanks for looking into this.

[thiemowmde]

Same for me. -2 for 3.0, but I agree that a replacement for this class that always confuses me would be nice. However, please be aware of what Jeroen always says and what I just started to say in #476 (comment): Help us focus on tasks on the board.

[Benestar]

Help us focus on tasks on the board.

There is a difference between you and me. I'm not employed at WMDE but do all of my work as a volunteer. Thus I can also decide on my own which things I like to tackle and work on. You have to know that I do those things in my freetime and on my own.

However, I talked to Lydia how I can coordinate my work better with your workflow and we decided to discuss how you will embed the work done by me into your sprints and reviews.

[JeroenDeDauw]

However, please be aware of what Jeroen always says and what I just started to say in #476 (comment): Help us focus on tasks on the board.

While in this case what I am saying amounts to the same position, I do not think this is correct phrasing of my general position which is not as crude. By which I'm asking you to not put words into my mouth.

[Benestar]

FYI, ByPropertyIdArray usage has just been reduced to only the one in ChangeOpClaim which however requires some more refactoring. Not putting this in to the 3.0 release is totally fine as having this does not hurt. However, maybe a deprecation notice could be added to discourage new uses of this class.

[JeroenDeDauw]

Ah, I did not realize we where so close to getting rid of it. In that case I do not object to have this in 3.0 and certainly not to deprecating. If the ChangeOpClaim refactor turns out to be hard, we can always just have the class in Wikibase Repository till that is done.

[Benestar]

I just realized that moving/reindexing of statements is currenlty neither supported nor implemented in the ui so this can perhaps be refactored quite easily by simply changing the wbsetclaim api module. If we manage to agree on a solution there, this patch can perhaps already go into 3.0

Edit: A possible solution can be found here.

[JeroenDeDauw]

Yeah, if the functionality which is implemented in a problematic way is not actually needed, then we should just remove it...

[JeroenDeDauw]

Needs rebase.

This class is not used in WikibaseDataModel (or DMS) anymore and has only a single usage in Wikibase.git.

[thiemowmde]

This is still used in ChangeOpStatement. I would love to get rid of this, but what is the replacement and the migration path? The proposed ByPropertyIdMap in #479 does have a very different interface. That's also my main issue with #479 and one of the reasons it's still not merged and released. An other reason is that it's more a service and probably belongs to DataModel Services.

I suggest to first mark ByPropertyIdArray as deprecated. Then work on ChangeOpStatement and replace it's usage of ByPropertyIdArray, possibly with code in private methods, or with a helper in repo. This way everything is in a single patch and can very easily be reviewed. Later the helper will be moved to DataModel Services. If all this is done, we can kill ByPropertyIdArray.

[JeroenDeDauw]

Closing this due to inactivity and need for redoing it anyway. Which seems plausible since there only is a single use left (ChangeOpStatement), where the needed code can likely just be inlined, or put in a smaller dedicated service.

[thiemowmde]

See wmde/WikibaseDataModelServices#157 for a proposal for such a smaller dedicated service.