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

Auto documentation #348

Merged
merged 11 commits into from
Oct 3, 2021
Merged

Auto documentation #348

merged 11 commits into from
Oct 3, 2021

Conversation

ahwagner
Copy link
Member

This PR adds auto-documentation functionality, class inheritance, and namespaced references to the YAML syntax.

Tests are passing locally on Python 3.9.4. There appears to be some error in python_jsonschema_objects that will need to be resolved. @reece any thoughts on that?

@ahwagner
Copy link
Member Author

ahwagner commented Sep 30, 2021

This problem with python-jsonschema-objects is being seen elsewhere, too. See issue cwacek/python-jsonschema-objects#223.

This is because the python-jsonschema-objects is using a module from the jsonschema library that was intended to be private, which was recently removed in the release to jsonschema 4.0.0. The maintainers of jsonschema have no intention of restoring or preserving the private module (see statement here), and the python-jsonschema-objects issue tracker has me doubtful that this will be addressed quickly.

@ahwagner
Copy link
Member Author

ahwagner commented Sep 30, 2021

8c8ff8e addresses the testing issue by locking the version of jsonschema used. This is a crappy solution but the best we can do until python-jsonschema-objects maintainers address the issue. We should consider doing the same over at ga4gh/vrs-python.

@ahwagner
Copy link
Member Author

Moving this PR to draft status. RTD isn't building the docs properly (see RTD Allele section from this branch), need to investigate.

@ahwagner ahwagner marked this pull request as draft September 30, 2021 13:09
schema/vrs.json Outdated
@@ -21,7 +21,7 @@
}
},
"MolecularVariation": {
"description": "A variation on a contiguous molecule.",
"description": "A :ref:`variation` on a contiguous molecule.",
Copy link
Contributor

Choose a reason for hiding this comment

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

@ahwagner while i think this is cool. I'm just wondering how description and other attribute string values that contain RST tags will impact other tools that use these fields to display the schema documentation (i.e. swagger). I may be missing the mark here and it really may not be a big deal, but are we thinking about how other commonly used tools will work with our enhanced RestructuredText syntax. I suppose the big value of RST and markdown in general is that it is still very legible even in an unrendered state.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is good feedback, will address.

@@ -70,24 +79,30 @@ definitions:

Allele:
description: >-
The sequence state at a Location.
The state of a molecule at a :ref:`Location`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that you used the term molecule instead of sequence here. Wouldn't sequence be more concise?

Copy link
Member Author

@ahwagner ahwagner Oct 3, 2021

Choose a reason for hiding this comment

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

This change was made to align the YAML with the documentation. But molecule probably is better anyways, as it also covers ChromosomeLocation.

Copy link
Contributor

@larrybabb larrybabb left a comment

Choose a reason for hiding this comment

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

this is a nice addition to the spec tooling. great job Alex.

@ahwagner ahwagner marked this pull request as ready for review October 3, 2021 18:09
@ahwagner ahwagner merged commit 6a9c467 into main Oct 3, 2021
@ahwagner ahwagner deleted the yaml-tools-rst branch October 3, 2021 18:12
@reece
Copy link
Member

reece commented Oct 3, 2021

@ahwagner: I was in the middle of reviewing the yaml tools PR when it merged. I have some concerns and questions. These changes seem big enough that they merited discussion before merging.

We definitely should not be putting RST in jsonschema. Markdown is supported (specifically by openapi clients), but RST isn't.

I also don't understand the rationale for the heritable business or how it's implemented. However, it appears to make schema.yaml no longer jsonschema. If true, I want to reconsider that as well.

@@ -400,7 +414,7 @@
]
},
"SequenceInterval": {
"description": "A SequenceInterval represents a span of sequence. Positions are always represented by contiguous spans using interbase coordinates.\nSequenceInterval is intended to be compatible with that in Sequence Ontology ([SO:0000001](http://www.sequenceontology.org/browser/current_svn/term/SO:0000001)), with the exception that the GA4GH VRS SequenceInterval may be zero-width. The SO definition is for an \"extent greater than zero\".",
"description": "A SequenceInterval represents a span of sequence. Positions are always represented by contiguous spans using interbase coordinates. SequenceInterval is intended to be compatible with that in Sequence Ontology (SO:0000001), with the exception that the GA4GH VRS SequenceInterval may be zero-width. The SO definition is for an \"extent greater than zero\".",
Copy link
Member Author

Choose a reason for hiding this comment

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

@ahwagner let's add links back in (markdown style) to the JSON-Schema artifact

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.

3 participants