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

build schema_validators dict for bluesky issue #1197 #79

Merged
merged 4 commits into from
May 23, 2019
Merged

build schema_validators dict for bluesky issue #1197 #79

merged 4 commits into from
May 23, 2019

Conversation

jklynch
Copy link
Contributor

@jklynch jklynch commented May 22, 2019

A dict 'schema_validators' is constructed of jsonschema.Validator instances. The intention is that this dict be imported by code wanting to validate JSON.

Motivation and Context

This change is in support of using jsonschema>3: bluesky/bluesky#1197.

jsonschema.validate() is currently used in bluesky but has been deprecated. The 'new' method is to extend a jsonschema validator class which takes about 4 lines of code. I suggest putting those 4 lines in event-model and caching one validator for each schema.

How Has This Been Tested?

@danielballan danielballan marked this pull request as ready for review May 23, 2019 17:40
@danielballan
Copy link
Member

This looks on track so me so I have gone ahead and marked it as ready for review so that CI will start running against it.

Please update all the places that we use jsonschema.validate , such as

jsonschema.validate(output_doc,

to use these validators instead. Please also update the requirements.txt to specify the minimum version of jsonschema that supports this new usage.

added schema_validators to __all__
replaced calls to jsonschema.validate() with schema_validators[].validate()
@jklynch
Copy link
Contributor Author

jklynch commented May 23, 2019

I think I have corrected those embarrassing oversights.

@danielballan
Copy link
Member

Haha, not at all! Very useful PR for Week 3 on the team!

Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready. I have taken it for a spin interactively and it holds up.

This should get at least one more pair of eyes before being merged, given its importance.

Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @jklynch!
Left a minor question regarding imports, otherwise good to go.


__all__ = ['DocumentNames', 'schemas', 'compose_run']
import jsonschema
import numpy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did those imports moved from their original places?

@jklynch
Copy link
Contributor Author

jklynch commented May 23, 2019 via email

@danielballan
Copy link
Member

I don't think we ever really decided whether we would adopt the "builtins, then external libs, then package-local libs" breakdown, but I have heard it argued for and I am happy to adopt it.

@danielballan
Copy link
Member

Per quick Slack check-in with @mrakitin we are indeed all on the same page that this is a good and reasonable compulsion. Thanks again @jklynch!

@danielballan danielballan merged commit b63ce2c into bluesky:master May 23, 2019
@mrakitin
Copy link
Member

I really like it though, as I expressed it some time ago myself. PyCharm normally does it for you "for free" ("Optimize imports" menu or something like that).

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.

None yet

3 participants