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

Update jsonschema dependency to >=4.15.0 (replacing >3) #281

Closed
wants to merge 2 commits into from

Conversation

sdruskat
Copy link
Member

@sdruskat sdruskat commented Aug 31, 2022

I'm in the process of using cffconvert to analyze validation errors in a dataset of CFF files.

jsonschema > 4 introduces a property jsonschema.exceptions.ValidationError.json_path that the currently bundled dependency (3.2.0) doesn't have. This property makes it really easy to find the offending field. I'd therefore suggest to bump the jsonschema version to >4.15.0 <5 as per this PR. (4.15.0 is known to work.)

All tests pass locally.

@sdruskat sdruskat requested a review from jspaaks August 31, 2022 13:00
@sdruskat sdruskat mentioned this pull request Aug 31, 2022
@sdruskat sdruskat changed the title Update jsonschema dependency to >4 (replacing >3) Update jsonschema dependency to >4.15.0 (replacing >3) Aug 31, 2022
@sdruskat sdruskat changed the title Update jsonschema dependency to >4.15.0 (replacing >3) Update jsonschema dependency to >=4.15.0 (replacing >3) Aug 31, 2022
@sdruskat
Copy link
Member Author

Error causes:

  • jsonschema 4.15.0 is not available for Python 3.6, perhaps retire support for 3.6?
  • The linting error thrown by prospector doesn't make sense to me. @jspaaks Do you know anything about this?
    It seems to refer to the linting of setup.cfg, specifically
[options.packages.find]
include = cffconvert, cffconvert.*

@jspaaks
Copy link
Member

jspaaks commented Aug 31, 2022

I want to look into this soonish, but initial thoughts are I'm positive about dropping support for Python 3.6 as that is end of life. A quick scan of jsonschema's CHANGELOG suggests that will mean increasing the minimum version of jsonschema to 4.0.0 (and maybe maximum version <5 as per @musicinmybrain 's PR). In principle I think we should set the limits of acceptability as lenient as possible, to facilitate people installing globally or even with --user option, since either of those can lead to clashing dependency versions, for example when they have other tools that require say jsonschema> 4.6.0.

@jspaaks
Copy link
Member

jspaaks commented Aug 31, 2022

Not sure what's going on with prospector (pylint really) but I wouldn't be surprised if I can make the error go away by raising the minimum required version on pylint

@sdruskat
Copy link
Member Author

sdruskat commented Sep 1, 2022

Not sure what's going on with prospector (pylint really) but I wouldn't be surprised if I can make the error go away by raising the minimum required version on pylint

Would be great, although I've used pylint 2.15.0 locally and still get the error, on Python 3.10.6 🤷.

@jspaaks
Copy link
Member

jspaaks commented Jul 5, 2023

Hi Stephan,

am I right in saying that cffconvert-the-cli's behavior is unchanged by updating from say 3.2.0 to 4.17.3? But when used as a library (as I suspect was your use case), you have better diagnostics, although you have to implement them yourself, probably by wrapping cffconvert's validate call in a try, catching the ValidationError, inspecting it for the presence of json_path, doing whatever you need to do with the path, then raising the original error again.

If my assumption is correct, I'd like to keep jsonschema's dependency range at >=3.x, <5 as it is after @musicinmybrain's PR #277 which I just merged. That way, more existing installations will be able to make use of cffconvert without clashes. Others such as yourself can then install jsonschema >4.15, which you'd get on a fresh install anyway, and then use that from the command line as-is. Later on I can add functionality that makes cffconvert actually use the json_path property in its output if the property is available.

Sidenote: PR #290 will also help in making the output more readable, at the command line at least, not so much when used as a library.

@jspaaks
Copy link
Member

jspaaks commented Feb 12, 2024

Superseded by #381

@jspaaks jspaaks closed this Feb 12, 2024
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 this pull request may close these issues.

2 participants