-
Notifications
You must be signed in to change notification settings - Fork 259
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
JSON output (yamllint -f json) #245
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code does json.dumps()
then json.loads()
then json.dumps()
again. Maybe it could be simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right;) I first did the ,\n
and []
-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep alphabetical order, some linters complain about this.
|
||
with RunContext(self) as ctx: | ||
cli.run(('-f', 'json', path)) | ||
print (ctx.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why printing this line?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.