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

Prism #99

Merged
merged 14 commits into from Jul 15, 2019

Conversation

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

commented Jul 11, 2019

No description provided.

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

jim-bo added some commits Jul 11, 2019

@jacoblurye
Copy link
Collaborator

left a comment

Looks like a solid start to me! I left a bunch of comments, many of them variations on these themes:

  • Can you add docstrings and/or return type hints to your helper functions throughout? Some of them are a little hard to decode at a glance.
  • Choosing variable names that are human-readable can help make this code more self-documenting.
  • There are a couple opportunities to do things more simply using library functions.

Also, can you run autopep8 on your code before merging? I ought to add code styling as part of the Travis checks.

Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved cidc_schemas/prism.py Outdated
Show resolved Hide resolved tests/test_prism.py Outdated
Show resolved Hide resolved tests/test_prism.py Outdated
Show resolved Hide resolved tests/test_prism.py
Show resolved Hide resolved tests/test_prism.py Outdated

jim-bo added some commits Jul 12, 2019

@jacoblurye
Copy link
Collaborator

left a comment

Thanks for taking the time to address my comments! The docstrings and variable name updates make a huge difference.

Just a few more comments, but with those resolved, this should be good to merge.

schema_path: str,
schema_root: str = SCHEMA_DIR,
return_validator: bool = False,
on_refs: Optional[Callable] = None) -> Union[dict, jsonschema.Draft7Validator]:

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Jul 15, 2019

Collaborator

The return type of this function as currently implemented is actually Union[dict, Tuple[dict, jsonschema.Draft7Validator], but maybe it would be better for the function to actually have this signature? In that case, the function would simply return validator if return_validator == True instead of the tuple validator.schema, validator.

Also, why is the function load_validator_and_schema below needed? Its functionality is a subset of load_and_validate_schema, and it has the same interface.

This comment has been minimized.

Copy link
@jim-bo

jim-bo Jul 15, 2019

Author Contributor

Agreed. I kept the signature, updated the return to just return validator and updated usage in code.

Deleted the superfluous function I introduced.

"""

# get just the json file
file_path = ref.split("#")[0]

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Jul 15, 2019

Collaborator

See comment: #99 (comment)

I think the code change I propose is worth making, because it allows us to rely on jsonschema's ref resolution logic, rather than rolling our own here.

This comment has been minimized.

Copy link
@jim-bo

jim-bo Jul 15, 2019

Author Contributor

Yeah I agree, I created a ticket to do this.

This comment has been minimized.

Copy link
@jacoblurye

jacoblurye Jul 15, 2019

Collaborator

Why not just make the change inline here in this PR? The code I provided works as a drop-in replacement -- all the tests still pass.

@jim-bo jim-bo merged commit f226618 into master Jul 15, 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 prism branch Jul 15, 2019

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.