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

Annotation of multiple properties broken since SMW 2.3 #1252

Closed
krabina opened this Issue Nov 6, 2015 · 7 comments

Comments

Projects
None yet
4 participants
@krabina

krabina commented Nov 6, 2015

The annotation of multiple properties described in [1]("assigning one value to multipel properties") is broken since SMW 2.3. It can be seen here: [2].

The annotation [[Testproperty1::Testproperty2::200]] does not work: Testproperty1 is set to "Testproperty2::200" instead of Testproperty1 and Testproperty2 both set to "200".

[1] https://semantic-mediawiki.org/wiki/Help:In-text_annotation
[2] http://sandbox.semantic-mediawiki.org/wiki/Testproperty

@krabina krabina changed the title from Annotation of multiple properties broken since SMW 1.23 to Annotation of multiple properties broken since SMW 2.3 Nov 6, 2015

@mwjames

This comment has been minimized.

Show comment
Hide comment
@mwjames

mwjames Nov 6, 2015

Contributor

he annotation [[Testproperty1::Testproperty2::200]]

@kghbln Do we really support this?? This is not tested at all because we explicitly fixed :: annotation use (#1066, #1075) so we know that the first part is a property and the rest belongs to a value [[DOI::10.1002/123::abc]] or [[Foo:::123]].

We have a test case [[Has page:::Page/::Foo:Bar]] [[Has text:::Text/::Foo:Bar]] [[Has url::http://example.org/::Foo:Bar]] where we specifically test that values contain "Page/::Foo:Bar", ":Text/::Foo:Bar", "http://example.org/::Foo:Bar"

If this is really a supported use case then we need to add a config setting for [[property::property::property::value]] to explicitly enable this as we have no way of telling what is or isn't a property. I also would make this setting an exception rather then a default because [[ ... :: ... ]]] is to declare one triple. (subject -> property -> value).

Contributor

mwjames commented Nov 6, 2015

he annotation [[Testproperty1::Testproperty2::200]]

@kghbln Do we really support this?? This is not tested at all because we explicitly fixed :: annotation use (#1066, #1075) so we know that the first part is a property and the rest belongs to a value [[DOI::10.1002/123::abc]] or [[Foo:::123]].

We have a test case [[Has page:::Page/::Foo:Bar]] [[Has text:::Text/::Foo:Bar]] [[Has url::http://example.org/::Foo:Bar]] where we specifically test that values contain "Page/::Foo:Bar", ":Text/::Foo:Bar", "http://example.org/::Foo:Bar"

If this is really a supported use case then we need to add a config setting for [[property::property::property::value]] to explicitly enable this as we have no way of telling what is or isn't a property. I also would make this setting an exception rather then a default because [[ ... :: ... ]]] is to declare one triple. (subject -> property -> value).

@krabina

This comment has been minimized.

Show comment
Hide comment
@krabina

krabina Nov 6, 2015

It worked in the previous version and it is documented in the user manual...

krabina commented Nov 6, 2015

It worked in the previous version and it is documented in the user manual...

@kghbln

This comment has been minimized.

Show comment
Hide comment
@kghbln

kghbln Nov 6, 2015

Member

This possibility to assign one value to multiple properties was there from the/my start (SMW 1.4 minimum) so this is indeed a regression. Personally I have never used this so it did not come to my mind when you added the new features but it may very well be out there and we cannot blame them to have used it. I'd opt for a configuration setting which defaults to "false". "true" would allow to migrate or willingly use it as an exception.

Member

kghbln commented Nov 6, 2015

This possibility to assign one value to multiple properties was there from the/my start (SMW 1.4 minimum) so this is indeed a regression. Personally I have never used this so it did not come to my mind when you added the new features but it may very well be out there and we cannot blame them to have used it. I'd opt for a configuration setting which defaults to "false". "true" would allow to migrate or willingly use it as an exception.

@kghbln kghbln added the bug label Nov 6, 2015

@krabina

This comment has been minimized.

Show comment
Hide comment
@krabina

krabina Nov 6, 2015

My current use case is to use it in an arraymap:
{{#arraymap:{{{Benutzer|}}}|,|x|[[Benutzer::Assigned to::Benutzer:x]]}}
This is so I can set a property of my choice alongside the property assigned to that is defined by Semantic Tasks. But I just tested it, and
{{#arraymap:{{{Benutzer|}}}|,|x|[[Benutzer::Benutzer:x]][[Assigned to::Benutzer:x]]}}
works fine as well...

krabina commented Nov 6, 2015

My current use case is to use it in an arraymap:
{{#arraymap:{{{Benutzer|}}}|,|x|[[Benutzer::Assigned to::Benutzer:x]]}}
This is so I can set a property of my choice alongside the property assigned to that is defined by Semantic Tasks. But I just tested it, and
{{#arraymap:{{{Benutzer|}}}|,|x|[[Benutzer::Benutzer:x]][[Assigned to::Benutzer:x]]}}
works fine as well...

@mwjames

This comment has been minimized.

Show comment
Hide comment
@mwjames

mwjames Nov 6, 2015

Contributor

Personally I have never used this so it did not come to my mind when you added the new features

Same here and that is why we have a test for the feature but not for "many to one" interpretation.

##
# The strict mode is to help to remove ambiguity during the annotation [[ ... :: ... ]]
# parsing process.
#
# The default interpretation (strict) is to find a single triple such as
# [[property::value:partOfTheValue::alsoPartOfTheValue]] where in case the strict
# mode is disabled multiple properties can be assigned using a
# [[property1::property2::value]] notation which may cause value strings to be
# interpret unanticipated in case of additional colons.
#
# @since 2.3
# @default true
##
$GLOBALS['smwgEnabledInTextAnnotationParserStrictMode'] = true;

Does this make sense? The effective change would be a one line but since I have to write tests that doesn't happen in the blink of an eye.

PS: I really hope we would have tests before fixing or adding features that would make life so much easier knowing that something is broken, now we have to work backwards.

Contributor

mwjames commented Nov 6, 2015

Personally I have never used this so it did not come to my mind when you added the new features

Same here and that is why we have a test for the feature but not for "many to one" interpretation.

##
# The strict mode is to help to remove ambiguity during the annotation [[ ... :: ... ]]
# parsing process.
#
# The default interpretation (strict) is to find a single triple such as
# [[property::value:partOfTheValue::alsoPartOfTheValue]] where in case the strict
# mode is disabled multiple properties can be assigned using a
# [[property1::property2::value]] notation which may cause value strings to be
# interpret unanticipated in case of additional colons.
#
# @since 2.3
# @default true
##
$GLOBALS['smwgEnabledInTextAnnotationParserStrictMode'] = true;

Does this make sense? The effective change would be a one line but since I have to write tests that doesn't happen in the blink of an eye.

PS: I really hope we would have tests before fixing or adding features that would make life so much easier knowing that something is broken, now we have to work backwards.

@JeroenDeDauw

This comment has been minimized.

Show comment
Hide comment
@JeroenDeDauw

JeroenDeDauw Nov 6, 2015

Member

If it makes you feel any better, I had no idea this feature existed either :)

Member

JeroenDeDauw commented Nov 6, 2015

If it makes you feel any better, I had no idea this feature existed either :)

mwjames added a commit that referenced this issue Nov 7, 2015

Merge pull request #1254 from SemanticMediaWiki/strict-mode
Add InTextAnnotationParserStrictMode flag, refs #1252

mwjames added a commit that referenced this issue Nov 7, 2015

Add InTextAnnotationParserStrictMode flag, refs #1252
By default (as of 2.3) a more strict parsing process is enforced to minimize
annotation interpretation issues.

$GLOBALS['smwgEnabledInTextAnnotationParserStrictMode'] can be set `false` to
loosen the strictness which may result in certain features not being
supported or can cause unanticipated misinterpretation in case of
additional colons.
@mwjames

This comment has been minimized.

Show comment
Hide comment
@mwjames

mwjames Nov 7, 2015

Contributor

@krabina Added your test case in p-0408 [#1252] intext to use non-strict mode.json with $GLOBALS['smwgEnabledInTextAnnotationParserStrictMode'] set to false to support [[Testproperty1::Testproperty2::200]].

[0] will be available with the next 2.3.1 release.

[0] https://semantic-mediawiki.org/wiki/Help:$smwgEnabledInTextAnnotationParserStrictMode

Contributor

mwjames commented Nov 7, 2015

@krabina Added your test case in p-0408 [#1252] intext to use non-strict mode.json with $GLOBALS['smwgEnabledInTextAnnotationParserStrictMode'] set to false to support [[Testproperty1::Testproperty2::200]].

[0] will be available with the next 2.3.1 release.

[0] https://semantic-mediawiki.org/wiki/Help:$smwgEnabledInTextAnnotationParserStrictMode

@mwjames mwjames closed this Jan 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment