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 ItemIdParser #503

Merged
merged 1 commit into from Sep 3, 2015

Conversation

3 participants
@thiemowmde
Copy link
Contributor

commented Jun 17, 2015

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2015

Can BasicEntityIdParser not be used for the described usecase?

@Benestar

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2015

I also think that a general parser should be used and then an instanceof ItemId check be made,

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2015

You can not pass an instanceof check via composition.

The basic parser accepts properties. The only way to construct the parser needed for the use cases I described is as follows:

new DispatchingEntityIdParser( array(
    ItemId::PATTERN => function( $idSerialization ) {
        return new ItemId( $idSerialization );
    }
) );

This is not nice for two reasons: It's odd to have a "dispatching" parser delegating to a single function. And having that closure repeated several times in a code base is not nice.

If you can think of a nicer way to create a parser that accepts a single entity type only, please tell me.

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2015

So this new parser is for a use case where property ids can occur but should not be accepted?

Can you point to the code where this would be needed or provide a minimal example of why an EntityIdParser is needed rather than simply doing new ItemId( $serialization )? I'm quite hesitant to +2 this without being able to think of an example.

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2015

https://github.com/wikimedia/mediawiki-extensions-Wikibase/blob/master/lib/includes/formatters/EntityLabelUnitFormatter.php#L44 is designed to accept any EntityIdParser, which is a mistake in my opinion because we know we should not accept Property URIs as units. But @brightbyte argues that formatters can and should resolve labels for all entity types. I disagree.

The parser proposed in this pull request seems to be the easiest solution.

An other solution would be to rework EntityLabelUnitFormatter to not require an EntityIdParser but do the URI to ItemId parsing and validation itself. Is this better?

An other solution would be an EntityIdParser that can be told which constructor it should call. But I don't like the idea of passing a PHP class name around as a string.

An other solution would be to add this as a feature to the existing SuffixEntityIdParser. But I'm not sure how. Same problem as above.

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2015

See https://gerrit.wikimedia.org/r/#/c/219016/ for an alternative proposal.

This does not make this addition obsolete. I still think this can be useful, even if we are not going to use it right away.

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2015

@thiemowmde can this be closed for now?

@thiemowmde thiemowmde force-pushed the itemIdParser branch from f8b0475 to a15b6a6 Sep 2, 2015

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

Rebased.

Why should this be closed? Is there a problem why the suggested class can not be in this component?

@thiemowmde thiemowmde force-pushed the itemIdParser branch from a15b6a6 to 7d8853b Sep 2, 2015

@thiemowmde thiemowmde referenced this pull request Sep 2, 2015

Closed

4.3.0 release #551

@Benestar

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2015

I believe the use case for this is valid and it makes sense to have this parser. Needs updates of the version though. Will we do a 4.4 release for this?

@thiemowmde thiemowmde force-pushed the itemIdParser branch from 7d8853b to c6c56ce Sep 3, 2015

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2015

In #551 (comment) I asked to add this to 4.3.0. Not sure why this was not possible.

Rebased again. Test added.

@thiemowmde thiemowmde force-pushed the itemIdParser branch from c6c56ce to b51b380 Sep 3, 2015

Benestar added a commit that referenced this pull request Sep 3, 2015

@Benestar Benestar merged commit 9530078 into master Sep 3, 2015

3 checks passed

Scrutinizer 7 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Benestar Benestar deleted the itemIdParser branch Sep 3, 2015

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2015

Any particular reason to release this as 4.4 today?

@thiemowmde

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2015

No, can wait some days. But thanks for asking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.