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 clear to list classes #649

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Add clear to list classes #649

merged 1 commit into from
Mar 7, 2017

Conversation

Benestar
Copy link
Contributor

@Benestar Benestar commented Feb 29, 2016

This is needed as a replacement for users of the holder interfaces to remove all elements from one of the given list classes.

Bug: T128363

*
* @since 5.1
*/
public function clear() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is Fingerprint::setAliasGroups, so we should not need this.

@JeroenDeDauw
Copy link
Contributor

Looks good. Could perhaps also test that an empty list does not change state. But not that important

@thiemowmde
Copy link
Contributor

thiemowmde commented Feb 29, 2016

@Benestar and I had a look in our code base. Do we really need these methods, and where? What we found:

  • EntityChangeFactory clears statements, descriptions and aliases for performance reasons. This is a hack, see T113468.
  • Deserializers are currently written in a way they first create an entity and then set all labels, descriptions and aliases in one step. This can be rewritten: For example, first deserialize all labels, construct a Fingerprint from that, and then construct an Item from that. No clear nor "setTermList" or something like that needed, only constructors.
  • There are "patchers" that manipulate terms in a very, very complicated way. This can be refactored. A label patcher, for example, should call removeLabel, setLabel, and nothing else. It should never create a new TermList and replace the old one. It's not a "patcher" if it does this.

@Benestar
Copy link
Contributor Author

On hold, should not block 5.1.0

@thiemowmde thiemowmde added this to the 6.0.0 milestone Mar 10, 2016
@thiemowmde
Copy link
Contributor

thiemowmde commented Mar 10, 2016

Rebased. Questions:

@thiemowmde thiemowmde removed this from the 6.0.0 milestone Mar 10, 2016
@Benestar Benestar changed the title Add clear to list classes [DNM] Add clear to list classes Mar 10, 2016
@thiemowmde thiemowmde changed the title [DNM] Add clear to list classes Add clear to list classes Feb 28, 2017
@thiemowmde thiemowmde added this to the 7.0.0 milestone Feb 28, 2017
@thiemowmde
Copy link
Contributor

What I wrote above is still true. There is still code that needs to clean these list classes. Refactoring all this code in a way that it does not need to do this is a lot of effort, and probably not really worth it.

I strongly supported #614 because it should not be possible to "repurpose" an existing entity and turn it into something completely different. There really should not be an Item::clear, on no entity type. But this here is very different. These are list classes. It's not really "repurposing" if code cleans a list of aliases.

Considering the odd fact that I just implemented a clean in wmde/WikibaseDataModelServices#157 I suggest to merge this patch now. Pinging @brightbyte for the sake of completeness.

This is needed as a replacement for users of the holder interfaces to
remove all elements from one of the given list classes.

Bug: T128363
@brightbyte brightbyte merged commit d0017a8 into master Mar 7, 2017
@brightbyte brightbyte deleted the clear branch March 7, 2017 13:27
@@ -342,6 +342,15 @@ public function filter( StatementFilter $filter ) {
}

/**
* Removes all statements from this list.
*
* @since 6.0
Copy link
Member

Choose a reason for hiding this comment

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

this should have been 7.0 (or maybe 6.4) I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Fixed in #719.

@thiemowmde thiemowmde mentioned this pull request Mar 7, 2017
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