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

Last bigone #94

Merged
merged 3 commits into from Jul 10, 2019

Conversation

Projects
None yet
2 participants
@jim-bo
Copy link
Contributor

commented Jul 10, 2019

Hey @jacoblurye can you give this change the first review.

jim-bo added some commits Jul 10, 2019

@jim-bo jim-bo requested a review from jacoblurye Jul 10, 2019

@jacoblurye
Copy link
Collaborator

left a comment

Looks good to me! Just one question to consider before merging.


schema_root = SCHEMA_DIR
schema_path = os.path.join(SCHEMA_DIR, "%s.json" % name)
schema = load_and_validate_schema(schema_path, schema_root)

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Jul 10, 2019

Collaborator

Since load_and_validate_schema internally builds a jsonschema.Draft7Validator equivalent to the one on line 24, maybe it should also return that built validator so that we don't need to rebuild it as we do here. Or, otherwise, there should be a method in the cidc_schemas package that returns a validator given a schema identifier, since that's a pretty central operation. Just marking this as a small future improvement -- not necessary for this PR.

This comment has been minimized.

Copy link
@jim-bo

jim-bo Jul 10, 2019

Author Contributor

Good point, we can refactor this in the future.

Show resolved Hide resolved schemas/aliquot.json

@jim-bo jim-bo requested review from dnunes88 and priti88 Jul 10, 2019

@jim-bo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@priti88 @dnunes88 please checkout this PR to familiarize yourself with the changes I've proposed. Comments welcome.

@jim-bo jim-bo merged commit 9c27051 into master Jul 10, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jim-bo jim-bo deleted the last_bigone branch Jul 10, 2019

jacoblurye added a commit that referenced this pull request Jul 10, 2019

Release changes on master (#96)
* Add installation from GitHub and release branch status (#93)

* Last bigone (#94)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Include schemas in the cidc_schemas package (#95)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Move schemas inside cidc_schemas

* Include schemas in distribution

* Update schema path logic to handle new location

* Update CLI commands in README

* Move schemas into cidc_schemas

* rename SCHEMA_ROOT to SCHEMA_DIR

* Add command to list all schemas from CLI

jacoblurye added a commit that referenced this pull request Jul 11, 2019

Release changes from master (#98)
* Add installation from GitHub and release branch status (#93)

* Last bigone (#94)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Include schemas in the cidc_schemas package (#95)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Move schemas inside cidc_schemas

* Include schemas in distribution

* Update schema path logic to handle new location

* Update CLI commands in README

* Move schemas into cidc_schemas

* rename SCHEMA_ROOT to SCHEMA_DIR

* Add command to list all schemas from CLI

* API updates (#97)

* Add SCHEMA_LIST as a constant

* Add option to get validation errors as a list of strings

* Convert assertions to validation errors

jacoblurye added a commit that referenced this pull request Jul 14, 2019

Release changes on master (#102)
* Add installation from GitHub and release branch status (#93)

* Last bigone (#94)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Include schemas in the cidc_schemas package (#95)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Move schemas inside cidc_schemas

* Include schemas in distribution

* Update schema path logic to handle new location

* Update CLI commands in README

* Move schemas into cidc_schemas

* rename SCHEMA_ROOT to SCHEMA_DIR

* Add command to list all schemas from CLI

* API updates (#97)

* Add SCHEMA_LIST as a constant

* Add option to get validation errors as a list of strings

* Convert assertions to validation errors

* Update type hints to reflect openpyxl loader interface (#100)

* Remove unused or unnecessary requirements (#101)

jacoblurye added a commit that referenced this pull request Jul 16, 2019

Release first pass at prism (#105)
* Add installation from GitHub and release branch status (#93)

* Last bigone (#94)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Include schemas in the cidc_schemas package (#95)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Move schemas inside cidc_schemas

* Include schemas in distribution

* Update schema path logic to handle new location

* Update CLI commands in README

* Move schemas into cidc_schemas

* rename SCHEMA_ROOT to SCHEMA_DIR

* Add command to list all schemas from CLI

* API updates (#97)

* Add SCHEMA_LIST as a constant

* Add option to get validation errors as a list of strings

* Convert assertions to validation errors

* Update type hints to reflect openpyxl loader interface (#100)

* Remove unused or unnecessary requirements (#101)

* Prism (#99)

* initial work on prism

* minor updates to support more complex loading of data

* first working commit of xlsx parser for whole blood shipping

* added partial working code for wes support, need to rebase to get working

* prism now works for wes

* forgot to add new requirements

* style updates to prism to address comments in #99

* updated test to address confusion on merge

* fixed broken type hint by changing function and updating code. remove extra library function

* simplified ref resolution for type coercion checking

jacoblurye added a commit that referenced this pull request Jul 18, 2019

Release file-generation (#108)
* Add installation from GitHub and release branch status (#93)

* Last bigone (#94)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Include schemas in the cidc_schemas package (#95)

* modified schema to embed assay into aliquot

* fixed typo

* renamed property in aliquot assays to assay to reflect usage

* Move schemas inside cidc_schemas

* Include schemas in distribution

* Update schema path logic to handle new location

* Update CLI commands in README

* Move schemas into cidc_schemas

* rename SCHEMA_ROOT to SCHEMA_DIR

* Add command to list all schemas from CLI

* API updates (#97)

* Add SCHEMA_LIST as a constant

* Add option to get validation errors as a list of strings

* Convert assertions to validation errors

* Update type hints to reflect openpyxl loader interface (#100)

* Remove unused or unnecessary requirements (#101)

* Prism (#99)

* initial work on prism

* minor updates to support more complex loading of data

* first working commit of xlsx parser for whole blood shipping

* added partial working code for wes support, need to rebase to get working

* prism now works for wes

* forgot to add new requirements

* style updates to prism to address comments in #99

* updated test to address confusion on merge

* fixed broken type hint by changing function and updating code. remove extra library function

* simplified ref resolution for type coercion checking

* added first prototype of filegen_wes (#107)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.