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

Fix :::: processing in InTextAnnotationParser, refs 1048, T32603 #1066

Merged
merged 1 commit into from Jun 25, 2015

Conversation

Projects
None yet
2 participants
@mwjames
Copy link
Contributor

mwjames commented Jun 18, 2015

Add support for [[Foo::Bar::Foobar]] and [[Foo:::0049 30 12345678]] (other examples include [[DOI::10.1002/(SICI)1522-2594(199911)42:5<952::AID-MRM16>3.0.CO;2-S]], [[Has text:::0049 30 12345678/::Foo]])

According to [0] the issue of ::: has been open since "MW 1.17.0 and SMW 1.5.6 or SMW 1.6.1, probably happening to SMW 1.6.0 and SMW <1.5.6 though not tested.".

[0] https://phabricator.wikimedia.org/T32603
[1] #1048

@mwjames

This comment has been minimized.

Copy link
Contributor Author

mwjames commented Jun 18, 2015

@kghbln As the originator of https://phabricator.wikimedia.org/T32603 it would be nice if you could give this some testing (with/without NS, with/without interwikilink etc.) to avoid any regression on behalf of the :: syntax. I only added a few examples to parser-04-02-intext-colon-edge-case-parsing.json from the top of my head but maybe you can think of other combinations that are worthwhile to test.

@jaakristioja If you feel up for the task you might want to test your [[Has IPv6::fc00:123:8000::/64]] example with this PR.

@mwjames mwjames added this to the SMW 2.3 milestone Jun 18, 2015

@mwjames mwjames force-pushed the double-colon branch from 7afce4a to 83a58b3 Jun 18, 2015

@mwjames

This comment has been minimized.

Copy link
Contributor Author

mwjames commented Jun 19, 2015

@SemanticMediaWiki/testers Any testers for this one because I'd like to close this issue rather quickly.

@kghbln

This comment has been minimized.

Copy link
Member

kghbln commented Jun 25, 2015

Just tested this with a couple of data types and SMW is now "behaving" much better. A note though:
20150625_smw_issue 1066
The first value is the directly stored on the second value is the queried value.

  • Code is working as expected as is stores and outputs as is.
  • Page and Temperature show the leading colon but do not store it. I think it should rather be "Has improper value for" like telephone number and URL, Date and Email
  • Telephone number, URL, Date and Email (last two not pictured) are working as expected and give a "Has improper value for"
  • Text is working as expected as it is stored and outputted as is. Here we see that MediaWiki takes the colon to indent.

@mwjames What do you think of Page and Temperature? However, I do not think that they are a holdup for merging this.

@mwjames

This comment has been minimized.

Copy link
Contributor Author

mwjames commented Jun 25, 2015

Just tested this with a couple of data types

👍

Page and Temperature show the leading colon but do not store it. I think it should rather be "Has improper value for" like telephone number and URL, Date and Email

The InTextAnnotationParser doesn't make any validation of the input/output as this is part of the DataValue ( _wpg, _tem etc.) validation process. Types like URL, Email have specific validations in place (doing a preg_match for valid values) and therefore most likely filter those "unfit" values and return with an error / "Has improper value for".

Page and temperature do not internally check for any specific value limitation therefore a preceding :is internally neglected or transformed to a valid (without :) value.

If we want to have a "Has improper value for" instead then the DataValue checks have to be extended such as:

For the Page type you will probably handle this gracefully since one can easily do use :Foo

{{#set:
 |Has page=:Foo
}}

without any issue.

For the Number, Temperature type ( SMWNumberValue) add something like:

    protected function parseUserValue( $value ) {
        // Set caption
        if ( $this->m_caption === false ) {
            $this->m_caption = $value;
        }

        if ( $value !== '' && $value{0} === ':' ) {
            $this->addError( wfMessage( 'smw-datavalue-first-colon-not-permitted', $value )->inContentLanguage()->text() );
            return;
        }
Fix :::: processing in InTextAnnotationParser, refs 1048, T32603
Add support for `[[Foo::Bar::Foobar]]` and `[[Foo:::0049 30 12345678]]`

https://phabricator.wikimedia.org/T32603
#1048

@mwjames mwjames force-pushed the double-colon branch from 83a58b3 to dfd89ae Jun 25, 2015

mwjames added a commit that referenced this pull request Jun 25, 2015

Merge pull request #1066 from SemanticMediaWiki/double-colon
Fix :::: processing in InTextAnnotationParser, refs 1048, T32603

@mwjames mwjames merged commit 9c143e2 into master Jun 25, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Scrutinizer 4 new issues
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mwjames mwjames deleted the double-colon branch Jun 25, 2015

@mwjames mwjames added the enhancement label Jun 25, 2015

mwjames added a commit that referenced this pull request Jun 28, 2015

Merge pull request #1075 from SemanticMediaWiki/error-check
Additional error formatting rules for ":", #1066 + #1074
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.