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
Version: wmf-deployment
Severity: major
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | hashar | T60279 Jenkins: Add a lint job for JSON | |||
| Resolved | Jdforrester-WMF | T63659 VisualEditor: modules/syntaxhighlight/rules/mysql.json has unescaped double quotes in a double quoted string | |||
| Resolved | • GWicke | T64490 Deploy repo has two invalid json files |
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
Change 107163 had a related patch set uploaded by Krinkle:
json-lint.php: recursively lint json files with PHP
(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
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
Change 113958 had a related patch set uploaded by Hashar:
Add json-lint.php command to -jslint jobs
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
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.htmlWill 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 ).
(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.
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.
Whether ideal for the long term or not, it was resolved by Antoine by including it in the jslint job.
For the long term, projects should include a jsonlint task in their own test pipeline.
For localisation json files in particular, see T66045#962750.