[DRAFT] #708 - Make pretty formatter output prettier

[sloanlance]

Fixes #708.

This makes the pretty output look better by making error header and footer lines equal lengths and to use Unicode characters in them. It also shows schema JSON in error messages as valid JSON, not Python data structure representations. (Mostly a difference in quote marks used for strings.)

For the error messages, I wanted to continue using the strings with str.format(). However, I couldn't find a clean way of using the left justification and padding that's available in the formatting micro-language. If the technique I used isn't satisfactory, I can try for another approach.

To get the schema instance to display as true JSON, I thought encoding the schema back to JSON was the best approach. Using newline with the separator and sorting keys seems to give close to the same output format as pprint.pformat(). The difference is that it ends up being a little prettier, because there are always line breaks, but in my testing, pprint only broke the lines when the schema was longer than 72 characters. I didn't test extensively, so I don't know how well the json.dumps() technique compares with pprint for lines that longer than 72 characters and aren't easily broken.

Please feel free to be critical with the code review. I'm very much open to updating my changes to fit with the style of the project.

[Julian]

Awesome thanks! I will take a closer look at this (possibly after you get to the tests) but just to save you from one wrong turn -- the "use JSON" desire I had should only apply to the CLI, not to general formatting of the exception -- in other words, we can't change it within e.g. ValidationError.__str__, otherwise it will apply everywhere.

We have some options there. The least coupled is to just construct a custom way to format the errors within the CLI (using all the attributes present on a ValidationError, same as ValidationError.__str__ does). But we also could factor out the body of ValidationError.__str__ into a parametrized helper function, and then call it in 2 different ways from ValidationError.__str__ and from the CLI.

But yeah want to save you from that otherwise you'll find you have to change lots of tests (and then find out later that I didn't mean that :D).

But yeah thanks for all you've done already! Seems like we're on the way here...

[Julian]

Cool. Yes, think you're getting to the same place I was describing -- I'd probably want it to just be a private helper function, so consider something like:

class _Error:
    ....

    def __unicode__(self):
        return _formatted_error(self, to_str=lambda thing: pformat(thing, width=72))

and then in the CLI you'd use _formatted_error(some_error, to_str=lambda thing: json.dumps(thing, indent=True, ...))

[Julian]

OK. I see, passing in a lambda for formatting will make the code more flexible. Would it be helpful to set pformat() as the default formatter in _formatted_error()? Then when called from unicode(), we wouldn't need to specify the formatter.

Sounds reasonable!

Also, I'm going to look at the documentation about running the tests locally, but if you can tell me off the top of your head the command for testing with my local environment, that'd be helpful. Tox is very cool, but it's more information than I need for initial testing.

Maybe this helps?:

python3.8 -m venv venv
venv/bin/python -m pip install ./ twisted
PYTHONPATH=./ venv/bin/trial jsonschema

(In theory you can replace twisted and trial there with any test runner, including pytest if you're more familiar with that, but the above is what I use.)

[codecov]

Codecov Report

Merging #712 into master will increase coverage by 0.02%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   96.02%   96.04%   +0.02%     
==========================================
  Files          17       17              
  Lines        2664     2681      +17     
  Branches      310      310              
==========================================
+ Hits         2558     2575      +17     
  Misses         87       87              
  Partials       19       19              

Impacted Files	Coverage Δ
jsonschema/exceptions.py	92.19% <75.00%> (+0.16%)	⬆️
jsonschema/cli.py	97.16% <100.00%> (+0.21%)	⬆️
jsonschema/tests/test_cli.py	99.09% <100.00%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ce8dd3...5185d44. Read the comment docs.

[Julian]

Hey! Sorry, I haven't really gotten a chance to look at the error you were hitting -- let me know though if you're still having difficulty -- replacing ljust with just the manual calculation sounds fine to me, but if it continues to be an issue I can have a look (maybe tomorrow)

[Julian]

Will have a look at this in a few hours I hope -- but are you sure this passes locally for you? It certainly fails here.

From glancing at the diff, it seems like you probably want native strings (i.e. no u prefix). stdout/stderr accepts native strings -- so str on both Py2 and Py3. Here it looks like you're giving it unicode on both, which yeah will blow up with that exception.

Will have some more specific guidance after having a look.

And thanks for the work so far it's much appreciated!

[Julian]

The CI failures are python-hyper/hyperlink#133 which is being resolved one way or another (either upstream or if not quickly then I'll shove a fix in here somehow)

[Julian]

sorry, to be clearer about what I mean there, since maybe that was too short -- that function doesn't use anything relevant to an instance, or to the class itself, so it should just be a global function, completely unattached to the class (and then you won't need neither @classmethod nor @staticmethod)

[Julian]

Generally I'd just leave it right alongside _PrettyFormatter -- it's fine to have a function just used in one place, putting it next to the class is just as good as having it on it. As you say, it's possible it'll be used again, if it is, it can move to a module that makes sense for more locations.

[sloanlance]

All CI checks except coverage passed. Automated coverage check isn't something I know much about, but I'd like to learn. According to the raw logs of the check, it looks like a configuration file for running the check is missing. Is that right?

I saw a coverage report from closer to the time I opened this PR. There have been several changes to my code since then, so it's probably out of date. Is there any value to using that report? I'm unsure how to interpret the report anyway. 🙈

Edit: I see that although the comment from the coverage report is 10 days old, the report itself is current. I'll look up the coverage documentation and try to understand what the report is telling us.

[Julian]

The failure there is codecov.io (which is the free service for hosting those reports) being flaky, likely nothing you did/changed.

I just re-ran the job, it's unfortunately common for it to barf. Let's see if it passes this time.

[sloanlance]

Yes, they ran this time. The only failure was to hit the 100% coverage target. Looks like there's an overall slight increase in coverage. Does this satisfy your requirements for merging, or would you like me to look into increasing coverage for code I added?

[Julian]

Yeah don't mind the coverage being confused not sure why codecov.io has begun to do that as well recently but you're fine there -- will give this another review I hope later today!

[Julian]

Awesome! Left a few minor comments but think we're almost there! Appreciated.

[Julian]

We're only using the formatter for the schema, not the instance here, so there's going to be inconsistency in the output.

Can we add a test that covers the instance too and then fix this to make sure the formatter is used for both?

[Julian]

Minor style: jsonschema follows PEP8 / generally avoids using backslashes for continuation. Can you switch these to use parentheses instead?

Though, I think this will be a lot clearer if we split it into _header_line and _non_header_line -- can you give that a shot? It should remove a bunch of the conditional behavior here.

[sloanlance]

I usually don't like backslashes, either. I got in the habit of using it because my coworkers do like them. I'll change this code.

[Julian]

In case it helps, I added a pre-commit config for you/others so you should be able to fix some of the style comments here by just running pre-commit (which will e.g. restore the import order).

[sloanlance]

In case it helps, I added a pre-commit config for you/others so you should be able to fix some of the style comments here by just running pre-commit (which will e.g. restore the import order).

What is the definition of the order of imports preferred for this project? I often use IntelliJ IDEA's reformatting feature. In addition to cleaning up lines of code, it reorganizes imports. Off the top of my head, I think the order is something like sections of core Python modules, third-party modules, and project modules, each section alphabetized.

[Julian]

What is the definition of the order of imports preferred for this project? I often use IntelliJ IDEA's reformatting feature. In addition to cleaning up lines of code, it reorganizes imports. Off the top of my head, I think the order is something like sections of core Python modules, third-party modules, and project modules, each section alphabetized.

It's exactly that in fact :), just actually alphabetized (some tools, e.g. isort by default, and it sounds like IntelliJ), will put from imports on the bottom rather than on the top, whereas "from" < "import".