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

Fix invalid JSON Schema #309

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Fix invalid JSON Schema #309

merged 2 commits into from
Aug 24, 2023

Conversation

aganders3
Copy link
Contributor

@aganders3 aganders3 commented Aug 23, 2023

This fixes an issue reported on Zulip by @seankmartin. Basically there was a small error in the generated manifest json schema:

❯ check-jsonschema --check-metaschema _schema.json
Schema validation errors were encountered.
  _schema.json::$.definitions.ConfigurationProperty.properties.type.anyOf[1].minItems: True is not of type 'integer'
  _schema.json::$.definitions.Draft06JsonSchema.properties.type.anyOf[1].minItems: True is not of type 'integer'
  _schema.json::$.definitions.Draft07JsonSchema.properties.type.anyOf[1].minItems: True is not of type 'integer'

The root of the problem is here, where min_items is set to True instead of an integer.
https://github.com/napari/npe2/blob/80ca5feb501dd25d36ecf1313f94d80366d25b8e/src/npe2/manifest/contributions/_json_schema.py#L24C3-L24C3

The first commit here adds a check of the schema in CI using check-jsonschema. Then the second commit should fix the underlying issue.

cc: @nclack

@aganders3
Copy link
Contributor Author

The first commit here adds a check of the schema in CI using check-jsonschema. Then the second commit should fix the underlying issue.

See this CI run for the expected new failure

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #309 (795336a) into main (80ca5fe) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #309   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines         2818      2818           
=========================================
  Hits          2818      2818           
Files Changed Coverage Δ
src/npe2/manifest/contributions/_json_schema.py 100.00% <100.00%> (ø)

@aganders3
Copy link
Contributor Author

Also note there is a double-import error when running python -m npe2.manifest.schema to generate the schema. I don't think it's causing problems, but I can open another issue for discussion on that.

<frozen runpy>:128: RuntimeWarning: 'npe2.manifest.schema' found in sys.modules after import of package 'npe2.manifest', but prior to execution of 'npe2.manifest.schema'; this may result in unpredictable behaviour

Copy link
Collaborator

@nclack nclack left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge tomorrow if there are no objections.

@nclack nclack merged commit b856dcd into napari:main Aug 24, 2023
@aganders3 aganders3 deleted the fix-schema branch August 24, 2023 15:36
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