Semanticmediawiki: Rework List format

[s7eph4n]

SMW 3.0 is coming up. I am reworking the list format continually updating this issue with the implemented changes.

Here is what I will do:

Separate separators for values, properties and result "rows".
E.g. three separators (sep=,, propsep=;, valsep=/) could produce the following result:

United States of America (Cities Washington / New York / Los Angeles; Neighbors Canada / Mexico), United Kingdom (Cities London / Glasgow / Manchester; Neighbors Ireland), France (Cities Paris / Marseille / Bordeaux; Neighbors Belgium / Luxemburg / Germany / Switzerland / Italy / Spain), and Germany (Cities Berlin / Frankfurt / Hamburg; Neighbors Poland / Czechia / Austria / Switzerland / France / Belgium / Luxemburg / Netherlands / Denmark)

This would also benefit the subformats (ol, ul, template, tree (in SRF))

A simpler logic of when which separator is used.
This is currently handled in ListResultPrinter.php and is rather confusing. I'd just reduce this to a default value ,, that may be overridden by parser function parameters. The default values should be defined in i18n messages.

Remove the final list separator (", and").
It has been gone for a while due to a bug and apparently nobody noticed.

Remove ? as prefix for template arguments (see #972).
Apparently nobody remembers why it was introduced in the first place.

Remove template arguments parameter.
Without the ? prefix for template parameters it is not needed anymore.

Make the template format an alias of the list format.
When the user supplies a parser function parameter template, then just assume they want it used, even for format=list. And inversely if format=template, but the parser function parameter template is missing or empty, just print a normal list.

Rename/standardize parameters to templates. E.g. all standard parameters start with a #:

• smw-resultquerycondition => #querycondition
• smw-resultquerylimit => #querylimit
• smw-resultqueryoffset => #resultoffset
• userparam => #userparam
• new: #rowcount
• new: #rownumber

Add class attribute to HTML elements.
To facilitate styling.

General code clean-up and removal of legacy/deprecated functionality
To make it easier to test the list format.

[jaideraf]

By the way, the CSV format could have some love too: https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/316 :sparkles:

[mwjames]

This proposal has my endorsement and to avoid me sitting in front of it maybe @kghbln @s7eph4n could build a working group to bring this feature home. The only requirement I would have is that by the end we have some integration tests [0] that cover most the new feature sets.

By the way, the CSV format could have some love too: #316 :sparkles:

If you find a sponsor (a person who implements, finalizes, and tests such request) then I can lend a hand.

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/tree/master/tests/phpunit/Integration/JSONScript#designing-an-integration-test

[s7eph4n]

Wow, I was not even aware of JSONScript until now. This looks great! Any idea how hard it would be to use it from SRF tests?

[mwjames]

I was not even aware of JSONScript until now. This looks great!

Well, it gets the job done and not in thousand years would I write all those tests in "pure" PHPUnit.

Any idea how hard it would be to use it from SRF tests?

This is how it is done in:

• SemanticCite: https://github.com/SemanticMediaWiki/SemanticCite/tree/master/tests/phpunit/Integration/JSONScript
• SemanticScribunto: https://github.com/SemanticMediaWiki/SemanticScribunto/tree/master/tests/phpunit/Integration/JSONScript and some superfluous comments from my side on this topic in https://github.com/SemanticMediaWiki/SemanticScribunto/issues/27
• Some easy intro video: https://youtu.be/7fDKjPFaTaY (plus comments https://github.com/SemanticMediaWiki/SemanticMediaWiki/pull/2348#issuecomment-304489093)

You basically have:

• JsonTestCaseScriptRunner as base class and for SMW core we derive
• JsonTestCaseScriptRunnerTest which makes all the dirty work and runs each single test from the JSON definition that is provided by a
• JsonTestCaseFileHandler instance

It goes without saying that we can only test the ParserOutput, for any JavaScript you need a headless browser and something like https://github.com/facebook/php-webdriver.

[s7eph4n]

Sounds good! I'll have a look.

Regarding JS CI testing I have a start in my filtered format rewrite. See https://github.com/s7eph4n/SemanticResultFormats/tree/filtered/formats/filtered#running-tests

[JeroenDeDauw]

To make it easier to subclass and test the list format.

Of course subclassing is almost inherently bad for code reuse. If you put effort into refactoring this, why not go for a SOLID design in which you favor composition over inheritance?

[mwjames]

Of course subclassing is almost inherently bad for code reuse. If you put
effort into refactoring this, why not go for a SOLID design in which you
favor composition over inheritance?

I understand the reasoning and in various cases it makes sense but is
not a rule to follow by hard and should be decided on a case by case
basis.

Of course, composition is recommended and yes, we agree that to avoid
code duplication to rely on the inheritance paradigm is bad practice
but don't kill the party before it has started.

The ResultPrinter class s the very representation of that breach, it
should be improved by (which is not part of this issue):

• ResultPrinter remove ContextSource dependency (let's greet the kitchen sink)
• Inject something like MessageLocalizer to avoid MW dep.
• Inject a TemplateParser as service (and is injected via
  AskParserFunction to retrieve the parser instance and avoid access to
  global $wgParser;) and remove most what happens in
  ResultPrinter::handleNonFileResult

Cheers

On 6/16/17, Jeroen De Dauw [e-mail] wrote:

To make it easier to subclass and test the list format.

Of course subclassing is almost inherently bad for code reuse. If you put
effort into refactoring this, why not go for a SOLID design in which you
favor composition over inheritance?

--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/2488#issuecomment-308945289

[Nikerabbit]

Tangentially related, but MediaWiki core has just got MessageLocalizer interface of its own that may cause confusion: https://phabricator.wikimedia.org/T162594

[s7eph4n]

Hmm, here's the problem: For something like [[Has number::{{#show:Some Page|Has some prop}}]] wrapping everything in HTML elements does not work. But the list format (or any other format) does not know that it was called for #show.
I see several possible solutions:

1. #show could use a new format plainlist ("subformat" of list that does not insert any HTML tags)
2. Introduce a new parameter plain for the list format that is set by default by #show
3. Add a logic to the list format where single values are never marked up

Opinions?

[mwjames]

Opinions?

I lean towards 1 because you isolate the representation to its own formatter which is the default for #show with no other format parameter being present.

2 means using the parameter plain to direct the output representation which we already do by declaring a format.

3 seems to make it a special case for single values only.

[s7eph4n]

Sounds reasonable. Thanks!

[s7eph4n]

Another issue: In the old 'ol' and 'ul' formats an attribute data-sortkey was inserted into the li elements, but I could not find it be used anywhere. E.g. exected output from p-0901.json:

                "to-contain": [
                    "<li data-sortkey=\"P\">Page/09/01/1</li>",
                    "<li data-sortkey=\"P\">Page/09/01/2</li>",
                    "<li data-sortkey=\"M\">Move/09/01/4</li>"
                ]

Do we still need that?

[mwjames]

old 'ol' and 'ul' formats an attribute data-sortkey was inserted into the li

I don't think we have a function (js) or printer that sorts ol/ul elements therefore I'm predicting that dropping data-sortkey should be fine.

. exected output from p-0901.json:

I found the original [0] and it seems there was no particular focus on the data-sortkey it just so happen that is was present in the selected format.

I think f-0104.json also checks for data-sortkey.

data-sortkey first appeared with https://github.com/SemanticMediaWiki/SemanticMediaWiki/blame/5bc50ceb0df0be51b38e03b950ad10196b75c10f/includes/queryprinters/ListResultPrinter.php#L273

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/pull/1086/files

[s7eph4n]

Original bug was

Enable .data( 'sortkey' ) (e.g. DEFAULTSORTKEY) for other SMW\ListResultPrinter content listener (Override the li element in case a sortkey is available)

https://static-bugzilla.wikimedia.org/show_bug.cgi?id=44275

I'll leave it out for the moment and if anybody complains we can still put it back in.

[s7eph4n]

Ok, I think it is pretty much feature-complete and tests run. I'll do some more cleanup and a few more tests and then open a pull request.

I summarized the changes in a gist.

The current state is available in branch listFormatRework, i.e. should be installable using composer with "mediawiki/semantic-media-wiki":"dev-listFormatRework" (or just check it out using git).

If there are no objections, I'd warn on the mailing list about the impending change, so people running master can prepare and then if everybody is happy in a few weeks we can merge.

[kghbln]

Currently we are testing another branch on sandbox.s-mw.o so I guess it is time to call the @SemanticMediaWiki/testers for assistance

[jaideraf]

I could'nt test it because when I update composer from "@dev" to "dev-listFormatRework"
I got that Fatal error: Uncaught Exception: .../SemanticMediaWiki/extension.json does not exist!
Any hint?

EDIT: I think it shouldn't be related (because SMW uses composer), but I use $wgExtensionDirectory to specify a different directory to my extensions.

[kghbln]

What happens when you go to extensions/SemanticMediaWiki/ and do a "git pull" followed by a "git checkout origin/listFormatRework". I find Composer completely inadequate when trying to switch between branches.

[jaideraf]

@kghbln What happens:

Warning: include(.../w/vendor/composer/../../extensions/SemanticMediaWiki/includes/storage/SMW_Store.php): failed to open stream: No such file or directory in .../w/vendor/composer/ClassLoader.php on line 444

Warning: include(): Failed opening '.../w/vendor/composer/../../extensions/SemanticMediaWiki/includes/storage/SMW_Store.php' for inclusion (include_path='.:/usr/local/share/pear71') in .../w/vendor/composer/ClassLoader.php on line 444

Warning: Class 'SMW\Store' not found in .../w/extensions/SemanticMediaWiki/src/Aliases.php on line 25

Warning: include(.../w/vendor/composer/../../extensions/SemanticMediaWiki/includes/storage/SMW_Store.php): failed to open stream: No such file or directory in .../w/vendor/composer/ClassLoader.php on line 444

Warning: include(): Failed opening '.../w/vendor/composer/../../extensions/SemanticMediaWiki/includes/storage/SMW_Store.php' for inclusion (include_path='.:/usr/local/share/pear71') in .../w/vendor/composer/ClassLoader.php on line 444

Fatal error: Class 'SMW\Store' not found in .../w/extensions/SemanticMediaWiki/src/SPARQLStore/SPARQLStore.php on line 29

[s7eph4n]

You will need to do a composer dump-autoload and a php maintenance/update.php.

[jaideraf]

@s7eph4n: I did, now:

Fatal error: Uncaught Exception: .../mw-REL1_30/extensions/SemanticMediaWiki/extension.json does not exist! in /w/includes/registration/ExtensionRegistry.php:99

Stack trace:

 #0 /w/includes/GlobalFunctions.php(120): ExtensionRegistry->queue('/home/jaideraf/...')
 #1 /w/extensions/SemanticMediaWiki/src/GlobalFunctions.php(220): wfLoadExtension('SemanticMediaWi...')
 #2 /w/LocalSettings.php(459): enableSemantics('wikincat.org')
 #3 /w/includes/WebStart.php(102): require_once('/home/jaideraf/...')
 #4 /w/index.php(40): require('/home/jaideraf/...')
 #5 {main} thrown in /w/includes/registration/ExtensionRegistry.php on line 99

[kghbln]

Perhaps this was branched before #1732 got merged?

[s7eph4n]

It was rebased a few days ago and the branch contains extension.json. See https://github.com/SemanticMediaWiki/SemanticMediaWiki/tree/listFormatRework .

[jaideraf]

Indeed, extension.json is present, but the code seems to look up based on "$wgExtensionDirectory", which, in my case, is not the default value "{$IP}/extensions".

[s7eph4n]

I just rebased the branch again, but I do not think that this is related to your issue. You could make sure by trying out the latest master.

[jaideraf]

Like I said, I think since SMW uses composer to install itself, it never used $wgExtensionDirectory before #1732. And MW is now looking for SMW extension.json outside {$IP}/extensions. I "solved" it by cloning SWM where $wgExtensionDirectory is pointing to (now I have two SMW directories: one at the standard path and the other at $wgExtensionDirectory).

Then I could test the list format, and everything seems fine to me. :+1: I just didn't test the parameters to templates (#userparam, #rowcount, etc.). I hope someone else could try them. Sorry for polluting the thread with issues that is not related.

[kghbln]

Thanks a bunch for testing.

I guess this and the answer is ...

[mwjames]

Ok, I think it is pretty much feature-complete and tests run. I'll do some more cleanup and a few more tests and then open a pull request.

@s7eph4n I'm happy to see you really could make it. We can merged it if you want and create a follow-up issue to be reminded of the outstanding tests before 3.0 or we wait until your done, as you like.

@jaideraf Thanks for the effort on testing. Would you mind creating an issue about your experience with wgExtensionDirectory in regards to what settings you used and which of them created some issues to evaluate whether the whole extension.json needs to be reverted or not.

[kghbln]

I'm happy to see you really could make it. We can merged it if you want and create a follow-up issue to be reminded of the outstanding tests before 3.0 or we wait until your done, as you like.

This may be the way to go, given the positive field test performed by @jaideraf This would also provide the option to use sandbox as a public testing instance for this rework and to add live examples.

[s7eph4n]

Ok, let's do that. Whoever is using master and relies on specifics of the old format (e.g. template params) will have to fix their wikipages and templates before updating.

Some SRF formats (tree, widget) will need an update as well.

[Nikerabbit]

I have ignored in translatewiki.net the messages which contain trailing spaces until they are fixed.

[kghbln]

I found the spot but did not find where the system messages are processed in the code so I leave this to @s7eph4n I guess the respective system messages should probably also be marked with {{Optional}} in the "qqq.json" file since not every language will need to localize them.

[s7eph4n]

In fact, most of them should be optional. Removing spaces however will not be possible.

E.g. the French put a space in front of colons (:), where English and Germans don't. I don't even want to start speculating how it is done in Arabic, Korean, Japanese, etc. So I'll replace them by &npsp;

[Nikerabbit]

You can use   or   as appropriate. All messages going through message cache will have these entities replaced with their actual values.

[krabina]

Started adding wikidocumentation at
https://www.semantic-mediawiki.org/wiki/Help:Plainlist_format

[kghbln]

Started adding wikidocumentation at https://www.semantic-mediawiki.org/wiki/Help:Plainlist_format

Thanks a lot. Today I also documented ol, ul, and list as well as doing fluff to plainlist. I just took me three hours. What still needs to be done is to redo the template format.

[kghbln]

I have completely redone the docu for plainlist. Also overhauled the template usage docu. This was an estimated 12 hours of work but finally everything is imho documented sanely with hopefully easy to grasp examples. The docu before provided evidence to be the result of a chicken stampede.