Skip to content

Add clear method to EntityDocument - #776

Merged
addshore merged 5 commits into
masterfrom
clearable
May 2, 2018
Merged

Add clear method to EntityDocument#776
addshore merged 5 commits into
masterfrom
clearable

Conversation

@jakobw

Copy link
Copy Markdown
Member

By letting entities clear themselves we can get rid of type-specific clearing logic in EditEntity, i.e.

  • getting rid of Property specific logic in EditEntity from re-adding the data type
  • not resetting the FormId counter for Lexemes

Related patch: https://gerrit.wikimedia.org/r/#/c/429792/

Task on Phabricator: https://phabricator.wikimedia.org/T192264

Comment threadtests/unit/Entity/ItemTest.php Outdated
}

public function clearableProvider() {
return array_merge( $this->instanceProvider(), [

@JeroenDeDauwJeroenDeDauwApr 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This mistake keeps haunting me :) I've been thinking this is an anti-pattern for years, but people are still copying my shitty code :)

I suggest to create dedicated tests:

testClearClearsSitelinks() {
$item = newItem();
$item->clear();
$this->suchAssert();
}
testClearClearsStatements() {
$item = newItem();
$item->clear();
$this->suchAssert();
}
testClearDoesNotClearId() {
$item = newItem();
$item->clear();
$this->suchAssert();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some clarification happened on IRC and I do not object to leaving the code as is, though think I'd be cool to ditch the instanceProvider call

Comment threadsrc/Entity/Clearable.php Outdated
*
* @license GPL-2.0+
*/
interface Clearable {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is this interface intended to be used? Will there be instanceof checks? Somehow it seems strange to me.

If this is only meant for Entities, calling it ClearableEntity is more obvious (even if it is in the Entity NS).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ClearableEntity does make it clearer (pun intended). Renamed.
Regarding its use: See the two bullet points and the gerrit patch I mention in the description here. Ideally we're getting rid of instanceof checks. A different question would be whether this should just be part of EntityDocument? Up for debate.

@JeroenDeDauw

Copy link
Copy Markdown
Contributor

Funny method. Got depracted, undeprecated, deprecated again, removed, and now added again.

Removal happened in 6.0: #614

No objection from my side to re-adding it, especially if there is no way to construct a new empty entity of the same type.

@JeroenDeDauw

Copy link
Copy Markdown
Contributor

Two points to consider:

  • Assigning new objects to the fields rather than calling clear on them breaks references. No idea if this is good or bad for this code
  • Maybe a version without mutation (more functional) is possible. Something like $entity->newWithSameId() or $entity->newWithId( $entity->getId() )

@addshoreaddshore changed the title Add Clearable interface for entitiesAdd clear method to EntityDocumentMay 2, 2018
@addshore
addshore merged commit 73e2db3 into masterMay 2, 2018
addshore pushed a commit that referenced this pull request May 2, 2018
* Add Clearable interface for entities
* Fix whitespace
* Rename to ClearableEntity
* Make clear part of EntityDocument.
* Improve ItemTest::testClear
@JeroenDeDauw
JeroenDeDauw deleted the clearable branch May 2, 2018 10:56
jakobw added a commit that referenced this pull request May 2, 2018
jakobw added a commit that referenced this pull request May 2, 2018
addshore pushed a commit that referenced this pull request May 2, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jakobw@JeroenDeDauw@addshore