JSON output (yamllint -f json) #245
Conversation
There was a problem hiding this comment.
Thanks for your PR @strowi!
At first glance it looks good, but lacks tests. For example, how does it behave when yamllint is run on multiple files, e.g. yamllint file.yaml other-file.yaml dir/? (I haven't tested, but it looks like it outputs incorrect JSON.)
Also please see some minor remarks below.
yamllint/cli.py
Outdated
|
|
||
| for problem in problems: | ||
| max_level = max(max_level, PROBLEM_LEVELS[problem.level]) | ||
| if no_warn and (problem.level != 'error'): | ||
| continue | ||
| if args_format == 'parsable': | ||
| print(Format.parsable(problem, file)) | ||
| elif args_format == 'json': | ||
| problems_json.append(json.loads(Format.json(problem, file))) |
There was a problem hiding this comment.
The code does json.dumps() then json.loads() then json.dumps() again. Maybe it could be simplified.
There was a problem hiding this comment.
Thx for taking the time. All the above are valid, maybe i was a bit hasty or this one got me too many headaches. (not a python pro...yet;)), and yes it's ugly.
But i am not sure how to make python return the json-object instead of the json-string and NOT break json-validity (there are descriptions containing double-quotes).
There was a problem hiding this comment.
No problem, your code looks globally good!
The first thing you could do is remove json.loads() from here and json.dumps() from Format. It would change anything, except lightening the code.
Another approach would be to directly print json.dumped strings + ',\n' in this line, and handle the starting [ and ending ] outside the loop.
There was a problem hiding this comment.
Oh right;) I first did the ,\nand []-thing but that wasn't really json-compatible, because on the last array-element it breaks json, which would've needed another exception..
I just pushed another update with the first-mentioned changes.
There was a problem hiding this comment.
Thanks @strowi, it looks better but not perfect yet.
I think you missed my comment:
At first glance it looks good, but lacks tests. For example, how does it behave when yamllint is run on multiple files, e.g.
yamllint file.yaml other-file.yaml dir/? (I haven't tested, but it looks like it outputs incorrect JSON.)
docs/quickstart.rst
Outdated
| "char": 2, | ||
| "description": "[warning] missing starting space in comment (comments)", | ||
| "code": "yamllint", | ||
| "name": "yamllint", |
There was a problem hiding this comment.
There are still code and name.
| @@ -25,6 +25,7 @@ | |||
| import shutil | |||
| import sys | |||
| import unittest | |||
| import json | |||
There was a problem hiding this comment.
Please keep alphabetical order, some linters complain about this.
|
|
||
| with RunContext(self) as ctx: | ||
| cli.run(('-f', 'json', path)) | ||
| print (ctx.stdout) |
| with RunContext(self) as ctx: | ||
| cli.run(('-f', 'json', path)) | ||
| print (ctx.stdout) | ||
| self.assertTrue(json.loads(ctx.stdout)) |
There was a problem hiding this comment.
I don't understand what you test here.
Again, can you please make a real check? For example:
with RunContext(self) as ctx:
cli.run(('-f', 'json', path))
expected_out = (
'[{"path":"%s",...' % path +
'"line":1,"char":1,...},'
'{"path":"file2",...'
'"line":1,"char":1,...},]\n')
self.assertEqual(
(ctx.returncode, ctx.stdout, ctx.stderr), (0, expected_out, ''))
I think you missed my comment: Yes, i did. But looking more into this, it seems i am at a loss.. Because of the way json is structured, the last line can't have a |
|
Please consider using the Codeclimate JSON syntax. Why come up with a proprietary JSON schema when a de facto standard exists? It would also be supported in GitLab. |
Hi @adrienverge,
here is the pull-request regarding json-output. Hope i got the coverage-test right. The problem was generating a really valid json for the whole report, not just a single problem.