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

[INFRA] Introduce metaschema #1787

Merged
merged 63 commits into from
Apr 19, 2024
Merged

Conversation

bendichter
Copy link
Contributor

@bendichter bendichter commented Apr 16, 2024

This PR contains:

  1. metaschema.json: A json schema document that specifies the entire schema language
  2. validate_schema.py: code for loading the schema, dereferencing it, and validating against the metaschema
  3. very minor adjustment to the actual schema, changing "deprecated" to "DEPRECATED", which appears to be the more dominant.
  4. tests ensuring that this correctly identifies erroneous additions to the schema

When I started this at the meeting in Seattle I did not expect the metaschema to be >700 lines, but there it is. I used definitions a bit to make it more concise. It's possible this could benefit from more definitions.

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.92%. Comparing base (53eafda) to head (5465f42).

❗ Current head 5465f42 differs from pull request most recent head 7d3445c. Consider uploading reports for the commit 7d3445c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1787      +/-   ##
==========================================
+ Coverage   87.79%   87.92%   +0.13%     
==========================================
  Files          16       16              
  Lines        1360     1375      +15     
==========================================
+ Hits         1194     1209      +15     
  Misses        166      166              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/metaschema.json Outdated Show resolved Hide resolved
src/validate_schema.py Outdated Show resolved Hide resolved
@bendichter
Copy link
Contributor Author

depends on #1790

@bendichter bendichter requested a review from Remi-Gau April 17, 2024 19:36
@bendichter
Copy link
Contributor Author

phew, sorry about all the commits here. It's ready for another round of review.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

OK so if I read this correclty the schema gets validated in CI wiht the metaschema, so the latter won't get out of synch.

Do you think you could add a couple of line of doc about all this?

@effigies
Copy link
Collaborator

Made a few small changes that were easier to do than suggest. Please have a look and let me know if you disagree.

@@ -0,0 +1 @@
../../../../src/metaschema.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

This gets resolved by setuptools during sdist build, so it's safe to use a symlink here.

) as file:
file.write(str(e))
# ValidationError does not have an add_note method yet
# e.add_note(f"See {file.name} for full error log.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently this is a 3.11+ builtin:

https://docs.python.org/3/library/exceptions.html#BaseException.add_note

It would be nice to display. Not sure if there's a way to hack this until 3.11 is our minimum.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Good with me. thanks @bendichter

@effigies effigies merged commit 3896ba5 into bids-standard:master Apr 19, 2024
23 of 24 checks passed
@effigies
Copy link
Collaborator

@bendichter Please add yourself to https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors if you aren't already on the contributors list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants