[CLI] Improve the overall console output (fix #623)

[RomainTT]

Fixes #623

This is just a suggestion, as there are plenty of ways to improve the visual. The current result is described below.

schema:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "type": "object",
    "required" : ["bar"],
    "properties": {
        "foo": {
            "type": "string"
        },
        "bar": {
            "type": "string"
        }

    }
}

data:

{
    "foo": null
}

Command line:

$ jsonschema -i data.json schema.json
———|ERROR|—————————————————
'bar' is a required property

Failed validating 'required' in schema:
    {'$schema': 'http://json-schema.org/draft-07/schema#',
     'properties': {'bar': {'type': 'string'}, 'foo': {'type': 'string'}},
     'required': ['bar'],
     'type': 'object'}

On instance:
    {'foo': None}
———|ERROR|—————————————————
None is not of type 'string'

Failed validating 'type' in schema['properties']['foo']:
    {'type': 'string'}

On instance['foo']:
    None

[Julian]

Thanks -- I'm definitely not opposed to a PR like this, but the "advantage" of the current ugly format is it's machine parsable in some sense.

I.e. you can grep it, and it's one line per error.

Possibly that's still not great, and there should be a json output (especially now that draft2019-09 has a standard JSON output representation).

I'm not opposed to having a human-readable one either as you're adding, but we should probably preserve the existing one, and leave room to add the new one (JSON output).

[RomainTT]

I agree with your comments. I applied a different strategy (leading to a lot of new code, I’m sorry) which should suits you more.

• The main thing is a human-readable -H option. If not turned on, the behaviour is the same than today. If turned on, every printed message is made more readable.
• I had to change the way json files are parsed, it is no longer made by argparse. It allowed to introduced several try: … except: to improve the readability of exceptions that can occure. These exception messages are also changed in function of -H.
• I added comments to this file (cli.py) because it became more complicated than it was.

Here is an overview of the result:

Default behaviour

$ jsonschema -i data.json -i data.good.json -i data.corrupt.json schema.json   
{'foo': None}: 'bar' is a required property
None: None is not of type 'string'
Failed to parse data.corrupt.json. Got the following error: Expecting property name enclosed in double quotes: line 2 column 2 (char 3)

Human readable

$ jsonschema -H -i data.json -i data.good.json -i data.corrupt.json schema.json
===[ERROR]===(data.json)===
'bar' is a required property

Failed validating 'required' in schema:
    {'$schema': 'http://json-schema.org/draft-07/schema#',
     'properties': {'bar': {'type': 'string'}, 'foo': {'type': 'string'}},
     'required': ['bar'],
     'type': 'object'}

On instance:
    {'foo': None}
===[ERROR]===(data.json)===
None is not of type 'string'

Failed validating 'type' in schema['properties']['foo']:
    {'type': 'string'}

On instance['foo']:
    None
===[SUCCESS]===(data.good.json)===
===[ERROR]===(data.corrupt.json)===
Failed to parse data.corrupt.json. Got the following error: Expecting property name enclosed in double quotes: line 2 column 2 (char 3)

And what if the schema is corrupted ?

$ jsonschema -i data.json schema.corrupt.json 
Failed to parse schema schema.corrupt.json. Got the following error: Expecting property name enclosed in double quotes: line 4 column 3 (char 84)

That one is not changed with -H

I think the path is clear to implement the JSON report, either with a new variable in the --error-format option, or with a brand new command-line option.

[Julian]

Thanks. It's definitely important we agree on approach.

Because of the third output format I mentioned (JSON), I'd rather see an option-based argument rather than a flag -- i.e. rather than --human, something like --output=plain|pretty|json. Which you don't need to add the third one in this PR, but then at least there's a logical place it can be added.

Also stylistically, I'd prefer we use helper functions, rather than comments -- if the function does too much usually to me that's a sign it should be split up, rather than have comments divide up sections of it. I try to keep functions to be 10 lines or less. The code though will likely change, so we can think about ways to do that perhaps after the above.

This is now also definitely enough code to bring up we should have tests for the new behavior. You'll notice also some of the existing ones are failing after the change. There's quite a bit here, so perhaps you can see how to unit test the changes to the output?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@053b64b). Click here to learn what that means.
The diff coverage is 97.5%.

@@            Coverage Diff            @@
##             master     #661   +/-   ##
=========================================
  Coverage          ?   95.04%           
=========================================
  Files             ?       18           
  Lines             ?     2621           
  Branches          ?      324           
=========================================
  Hits              ?     2491           
  Misses            ?      107           
  Partials          ?       23

[RomainTT]

I am really sorry that I had to make some force-pushes, but I had an identity issue and cannot let that as it was.

I finally managed to update the existing tests to make them pass. Now I am going to create some new ones to complete the coverage.

[RomainTT]

Hi @Julian ! I finally reached a version I am happy with.

It implements what you suggested, it is fully retrocompatible, and lets room for further improvements. The console output is still the same than in my previous comment.

I fixed the existing tests and added some new ones.

The coverage does not reach 100% because I don’t think we can test the sys.stdin input in a unittest test suite, and for the rest of the coverage I don’t really understand why it is not reached. What do you think ?

If everything is ok for you, I am done with it.

[Julian]

Thanks! Well done, much appreciated.

I left some initial comments, including a minor approach one, but we are definitely getting close.

I'm going to try to read things a bit more carefully over the weekend but wanted to get a first pass out immediately.

Thanks again for seeing this through!

[Julian]

For better or worse this module is still public (which #600 will fix) -- so we should be careful not to add any more public things to it. Can you rename this to be private?

But also -- I think you have the right idea here of an output writer, but if you use object polymorphism, things get even nicer.

I.e., rather than have every method have if whatever == "pretty": do_something_pretty(), have _PrettyOutputWriter and _PlainOutputWriter with the same interface, and then just select which writer is used based on the command line flag.

Lemme know if that makes sense to you, otherwise happy to elaborate.

[RomainTT]

Of course that is much better ! I just didn’t think about it. But it is definitely the kind of things I like to write in Python.

[Julian]

Can we avoid abbreviations? Here write_valid_error is particularly confusing, I assume this means write_validation_error?

[RomainTT]

Right. This is a confusing abbreviation, I did not notice it myself sorry.

[Julian]

So! After quite a bit of noodling, some of which not directly related to the PR, but this file needed some love, I'm going to merge.

There's still a bit more work I want to do to clean some of these files up but your contribution is very much appreciated!

Thank you for jumping in! Getting these things off the ground is 90% of the trouble typically, so if it weren't for you it wouldn't have happened :).

[RomainTT]

@Julian nice !!!

Thank you very much for your support !

I have another PR coming for you 😚