Skip to content
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

cleanup of pretty output renderer #708

Closed
4 tasks done
sloanlance opened this issue Jul 22, 2020 · 6 comments
Closed
4 tasks done

cleanup of pretty output renderer #708

sloanlance opened this issue Jul 22, 2020 · 6 comments

Comments

@sloanlance
Copy link

sloanlance commented Jul 22, 2020

Based on a conversation started in the comments of #706, clean up the appearance of the new pretty output renderer. Pertinent points from the conversation…

  • Fix length of the output dividers (current length is arbitrary; the length of = and - lines do not match)
  • Change to Unicode box characters from ASCII
  • Add vertical space between each error
  • Change exception messages from Python object reprs (with single-quoted strings) to true JSON (with double-quoted strings)
Details I think more important than adding even more output renderers is essentially cleaning up the one new one that was added. Specifically, here's some example output from the new pretty renderer:
⊙  printf '7' | jsonschema --output pretty <(printf '{"minimum": 9, "multipleOf": 3, "not": {"const": 7}}')

===[ValidationError]===(<stdin>)===

7 is less than the minimum of 9

Failed validating 'minimum' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
-----------------------------
===[ValidationError]===(<stdin>)===

7 is not a multiple of 3

Failed validating 'multipleOf' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
-----------------------------
===[ValidationError]===(<stdin>)===

{'const': 7} is not allowed for 7

Failed validating 'not' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
-----------------------------

where some things that rub me a bit wrong visually are:

  • the width of the ascii dividers (right now it's a bit arbitrary and you can see it doesn't match between the = and - lines -- particularly so if it's a SchemaError vs a ValidationError)
  • not using unicode box characters
  • needing a bit of vertical space between each error
  • the fact that, though it's not obvious there, in the exception message, what's there is reprs of Python objects. Even though usually that's what I want shown because jsonschema normally deals with Python objects, not JSON, here that's not the case -- here in fact the reverse is true -- the CLI takes JSON, and does the deserialization itself. So when showing errors back to users of the CLI, it should show JSON, not Python objects.
@sloanlance
Copy link
Author

sloanlance commented Jul 24, 2020

What do you think of this output, @Julian, with divisions padded to a width of 79 characters per line?

╒══[ValidationError]═══(<stdin>)══════════════════════════════════════════════╕
7 is less than the minimum of 9

Failed validating 'minimum' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
└─────────────────────────────────────────────────────────────────────────────┘

╒══[ValidationError]═══(<stdin>)══════════════════════════════════════════════╕
7 is not a multiple of 3

Failed validating 'multipleOf' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
└─────────────────────────────────────────────────────────────────────────────┘

╒══[ValidationError]═══(<stdin>)══════════════════════════════════════════════╕
{'const': 7} is not allowed for 7

Failed validating 'not' in schema:
    {'minimum': 9, 'multipleOf': 3, 'not': {'const': 7}}

On instance:
    7
└─────────────────────────────────────────────────────────────────────────────┘

@Julian
Copy link
Member

Julian commented Jul 24, 2020

Very nice! Definitely an improvement! Thumbs up from me.

@sloanlance
Copy link
Author

Perhaps off-topic: The error message '{'const': 7} is not allowed for 7' seems odd. Unlike the other two messages in this example, the value being tested (7) comes at the end of the statement and the JSON code seems unnecessary. I think a better error message would be simply 7 is not allowed.

If you agree, should this be a new issue, or would it be OK for me to change it as part of this issue and PR?

@Julian
Copy link
Member

Julian commented Jul 25, 2020

I agree on the awkwardness -- let's tackle it in a new issue -- the thing that is a bit complicated is that what's involved there is less about const and more about not -- not needs a message suitable to invert any condition (schema). So in general it's not easy to have that / know what schema you're inverting. Here of course when it's const it's obvious what better looks like, but not won't know that in general. But yeah definitely if we can find a way to not terribly overcomplicate things I'm all for it.

sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 26, 2020
Unicode box characters, true JSON output, (slightly) more space between each error, etc.
@sloanlance
Copy link
Author

sloanlance commented Jul 26, 2020

I'll open a new issue for the "not" error message.

sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 27, 2020
Only use JSON formatting for exception messages when explicitly requested.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 28, 2020
As clarified in PR, not all exception output needs to use valid JSON.  Add a message formatter method to the exception class and pass in a formatter lambda/function when needed.  Updated tests with the new expected output format.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 28, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 29, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 29, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 29, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Jul 29, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 3, 2020
These are not needed, as I expected.  They may even contribute to the "buffer interface" errors.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 3, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 3, 2020
From one version of Python to another, the "type" that appears in brackets may be of differing lengths.  (E.g., `JSONDecodeError` in Python 3, but `ValueError` in Python 2.)  That would make the length of the Unicode bar different under each version of Python.  When these tests were corrected to work under Python 2, they wouldn't work under Python 3 and vice versa.  Therefore, it's simpler to break the single assertions into two shorter ones that use very little of the bar characters.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 3, 2020
For Python interpreters that don't treat multibyte Unicode characters as single characters, add some code to manually calculate the number of bar characters required to reach the desired line length.  This code is confined to a conditional block and may be removed when support for older interpreters is no longer needed.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 4, 2020
With any luck, it will resolve the coverage issues, too.
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 4, 2020
sloanlance added a commit to sloanlance/jsonschema that referenced this issue Aug 4, 2020
@Julian
Copy link
Member

Julian commented Jun 7, 2022

Superceded by check-jsonschema which is now the official (and more-featureful) CLI. Anyone interested in this is invited to have a look there (and file any issues necessary).

@Julian Julian closed this as completed Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants