-
Notifications
You must be signed in to change notification settings - Fork 36
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
V1 release #49
V1 release #49
Conversation
…style conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pieces (mol, input, output, w/output incl. input) are orthogonal enough that separately named/versioned schemas are a good idea.
"name": "QC_JSON", | ||
"version": "0.1.dev", | ||
"url": "http://schema_host.org/schemas/0/something.schema", | ||
"$schema": "http://json-schema.org/draft-04/schema#", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use draft-04, not 06? from my uninformed view, the 06 changes (http://json-schema.org/draft-06/json-schema-release-notes.html) look pretty good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe we use anything in 6 at the moment, so might as well back down the compatibility requirements until we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we'll just be on-guard for any field names involving id
.
qcschema/dev/dev_schema.py
Outdated
"url": "http://schema_host.org/schemas/0/something.schema", | ||
"$schema": "http://json-schema.org/draft-04/schema#", | ||
"name": "qc_schema_input", | ||
"version": "dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't hurt to make this 1.dev
, so at least one knows what version you're approaching. scrupulously, this should be 1.devN
where N
is git commits past 0
tag, but 1.dev
will do. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fixed. I didn't add versioner or the like to keep it with the simple versioning philosophy.
"description": "The MolSSI Quantum Chemistry Schema", | ||
"type": "object", | ||
"properties": { | ||
"molecule": molecule.molecule, | ||
"schema_name": { | ||
"type": "string", | ||
"pattern": "\W*(QC_JSON)\W*" | ||
"pattern": "\W*(qc_schema)\W*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why qcschema
in docs and dir structure and qc_schema
elsewhere? I'm glad of the change from QC_JSON
, both in caps and name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qcschema
is canonical Python for modules (I dont think this is in the docs?) while qc_schema
is the keyword snake case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the distinction – makes much more sense now. (reference to docs was the sphinxproj name.)
"name": "qc_schema_molecule", | ||
"version": "dev", | ||
"description": "The MolSSI Quantum Chemistry Molecular Schema", | ||
"type": "object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
below at https://github.com/MolSSI/QC_JSON_Schema/pull/49/files#diff-fb40813be873a1ea003de289cb1f9373R59, length isn't really (nat,)
, right for bonds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, fixed.
@@ -1,6 +1,6 @@ | |||
{ | |||
"schema_name": "QC_JSON", | |||
"schema_version": 0, | |||
"schema_name": "qc_json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't these have to be qc_schema
or (qcschema
, I'm confused)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are input failures and apparently not well enough tested. Fixed.
"name": "QC_JSON", | ||
"version": "0.1.dev", | ||
"url": "http://schema_host.org/schemas/0/something.schema", | ||
"$schema": "http://json-schema.org/draft-04/schema#", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we'll just be on-guard for any field names involving id
.
@@ -1,6 +1,6 @@ | |||
{ | |||
"schema_name": "QC_JSON", | |||
"schema_version": 0, | |||
"schema_name": "qc_json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs renaming
@@ -1,6 +1,6 @@ | |||
{ | |||
"schema_name": "QC_JSON", | |||
"schema_version": 0, | |||
"schema_name": "qc_json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs renaming
"description": "The MolSSI Quantum Chemistry Schema", | ||
"type": "object", | ||
"properties": { | ||
"molecule": molecule.molecule, | ||
"schema_name": { | ||
"type": "string", | ||
"pattern": "\W*(QC_JSON)\W*" | ||
"pattern": "\W*(qc_schema)\W*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the distinction – makes much more sense now. (reference to docs was the sphinxproj name.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
lgtm, too. Don't think I have anything to add that would be required in v1. Also, agree that separate/modular schema would be best and easier to manage. |
Description
This PR aims to provide the necessary infrastructure and schema improvements to have a version 1 release, see #47. This version 1 is by no means feature complete, but aims to give a scaffold to build off of. Enumerated changes:
QC_JSON
toqc_schema
to match style choices and current naming directions.qc_schema_input
,qc_schema_output
,qc_schema_molecule
to better reflect what each of these objects are.Questions
TODO
Status