Last modified: 2014-11-20 01:37:38 UTC
We're seeing more and more JSON files entering our repos, and there is an RfC in discussion[1] to move to JSON files for our localisation. Please add standard JSON linting when patch sets that have Jenkins jobs configured. [1] https://www.mediawiki.org/wiki/Requests_for_comment/Localisation_format
Timo thoughts? Should we incorporate that in the jslint job that runs jshint ?
Any updates on this yet?
The jslint job is already present for extension and is triggered by Zuul whenever a .jshint*, *.js or *.json file is modified. A culprit is that by default jshint does not parse .json files, so we want to instruct it to parse such files using: jshint --extra-ext json .
json files requires double quotes which jshint complains about. So I guess we need a different job that would use jshint with a specific configuration.
(In reply to comment #4) > json files requires double quotes which jshint complains about. So I guess we > need a different job that would use jshint with a specific configuration. If we're using JSON, we might as well do it right, so I guess those complaints are justified?
(In reply to comment #5) > (In reply to comment #4) > > json files requires double quotes which jshint complains about. So > > I guess we need a different job that would use jshint with a specific > > configuration. > > If we're using JSON, we might as well do it right, so I guess those > complaints > are justified? Oh, wait. Yes, of course, Antoine :).
I have an example of invalid JSON that got merged, and caused issues in the L10n process: Line 215 of https://gerrit.wikimedia.org/r/#/c/106460/1/data/i18n/qqq.json has a trailing comma that, when a JSON linter is there, should have blocked the merge of that patch set.
I guess one can write a basic lint script that would wrap around PHP json_decode() ? If we can decode it, I guess we are safe :]
Change 107163 had a related patch set uploaded by Hashar: json-lint.php: recursively lint json files with PHP https://gerrit.wikimedia.org/r/107163
Change 107163 had a related patch set uploaded by Krinkle: json-lint.php: recursively lint json files with PHP https://gerrit.wikimedia.org/r/107163
(In reply to comment #4) > json files requires double quotes which jshint complains about. So I guess we > need a different job that would use jshint with a specific configuration. Have you verified that it warns for double quotes in *.json files as well? I don't think JSHint will parse json files exactly the same as js files, even if explicitly told to via ` --extra-ext json` because a plain JSON object is not a valid JS statement. Meaning, if you feed { "foo": "bar" } to a javascript parser as a statement, it will not be an object (it will be a block statement with a label foo and a statement with an unassigned string "bar"). As such, using jshint to parse json files would either be 1) pointless, as it would be validated as javascript instead of json, likely causing all kinds of false positives not even related to coding or quoting style or 2) JSHint recognises it as a JSON file and therefore one would expect it to not warn for double quotes.
(In reply to comment #11) > Have you verified that it warns for double quotes in *.json files as well? Yes I did (comment #4) which prompted me to repurpose this bug in writing a wrapper around PHP json_decode() which I did in Gerrit change #107163. I guess jshint would have the same issue with trailing comma in arrays (none in json) or keys having to be enclosed by quotes.
Change 107163 merged by jenkins-bot: json-lint.php: recursively lint json files with PHP https://gerrit.wikimedia.org/r/107163
Change 113958 had a related patch set uploaded by Hashar: Add json-lint.php command to -jslint jobs https://gerrit.wikimedia.org/r/113958
I have run the json linter on all extensions and three files are not understood by PHP json_decode() :-( hashar@gallium:~/extensions$ /srv/deployment/integration/slave-scripts/bin/json-lint.php . ./TemplateData/spec.templatedata.json: json decode error. ./VisualEditor/modules/ve-mw/i18n/qqq.json: json decode error. ./VisualEditor/modules/syntaxhighlight/rules/mysql.json: json decode error. hashar@gallium:~/extensions$
Change 114464 had a related patch set uploaded by Hashar: json syntax error with escaped single quotes https://gerrit.wikimedia.org/r/114464
Change 114464 merged by jenkins-bot: json syntax error with escaped single quotes https://gerrit.wikimedia.org/r/114464
Mailled wikitech-l about it http://lists.wikimedia.org/pipermail/wikitech-l/2014-March/075092.html Will be done next Monday: March 17th.
(In reply to Antoine "hashar" Musso from comment #18) > Mailled wikitech-l about it > http://lists.wikimedia.org/pipermail/wikitech-l/2014-March/075092.html > > Will be done next Monday: March 17th. Was it? Another example: https://gerrit.wikimedia.org/r/#/c/121910/ All extensions should have JSON files checked for validity (locally I use a simple json_verify < $FILE.json ).
*** Bug 63293 has been marked as a duplicate of this bug. ***
(In reply to Nemo from comment #19) > > Will be done next Monday: March 17th. > > Was it? Seems not. Setting severity from Siebrand's duplicate.
I forgot to deploy https://gerrit.wikimedia.org/r/#/c/113958/ last monday. Updating jobs now.
Change 113958 merged by jenkins-bot: Add json-lint.php command to -jslint jobs https://gerrit.wikimedia.org/r/113958
All 275 jobs updated.
Re-opening. I just managed to submit a patch ( https://gerrit.wikimedia.org/r/#/c/147754/ ) with a rather stupid json error, and jenkins did not complain.
(In reply to Bawolff (Brian Wolff) from comment #25) > Re-opening. I just managed to submit a patch ( > https://gerrit.wikimedia.org/r/#/c/147754/ ) with a rather stupid json > error, and jenkins did not complain. Its been pointed out to me that the jslint job (which is non-voting on TimedMediaHandler) is what verifies json files (Although https://integration.wikimedia.org/ci/job/mwext-TimedMediaHandler-jslint/497/consoleText doesn't seem to say anything about an i18n file failing). I'd like to suggest that the job be split. As missing a coding convention and a fatal error are rather different types of issues.
(In reply to Bawolff (Brian Wolff) from comment #26) > (In reply to Bawolff (Brian Wolff) from comment #25) > > Re-opening. I just managed to submit a patch ( > > https://gerrit.wikimedia.org/r/#/c/147754/ ) with a rather stupid json > > error, and jenkins did not complain. > > Its been pointed out to me that the jslint job (which is non-voting on > TimedMediaHandler) is what verifies json files (Although > https://integration.wikimedia.org/ci/job/mwext-TimedMediaHandler-jslint/497/ > consoleText doesn't seem to say anything about an i18n file failing). I'd > like to suggest that the job be split. As missing a coding convention and a > fatal error are rather different types of issues. I agree it shouldn't be part of the jslint job. But I don't think we should change that for the time being. The jslint job in general is an older one that is being phased out by having projects adopt Grunt instead. Where they can add jshint, jscs, banana-checker, and (if they have non-i18n json files) json-lint to their pipeline. TMH repo should be updated by adding the appropriate jshint config and ignores so that their non-voting exemption may be lifted. Because right now, obvious syntax errors in javascript files aren't caught, either. Which is just as important as json syntax errors.
