Closed jgrethe closed 6 months ago
@athril I think this looks good - all checks passed.
@athril Went through the comments. Some things can be included in future releases. We can setup tickets for those things (e.g. Jupyter notebook, etc). But that should not hold up this version.
@athril Went through your review. Replied to your comments and also added ticket for future features (e.g. Jupyter notebook). Most of comments were about the examples file and that should stay as is as per the comments. Also had some comments about setting URL (also answered in comments) and about the getURL function (works now - so any updates/improvements can be added in future updates).
@athril @hsorby Python notebook added based on README.io tutorial so that they match. Also made suggested change to the metadata client and placed URIs into init. Removed the example python file as this is now in a notebook - so all comments on the file are not applicable anymore
@hsorby All changes suggested from last review have been committed
It seems that tests/test_zinc.py issues may not be related to your pull request. Still there seem to be no tests included in this pull request.
@athril We have local tests we run with an API key (not to be shared via Github). Where can such a private key for the configuration file be placed (where it is not made public)?
@athril We have local tests we run with an API key (not to be shared via Github). Where can such a private key for the configuration file be placed (where it is not made public)?
Is there a chance to cover the code on Github, @jgrethe? I would rather avoid API calls if not absolutely necessary.
You could potentially mock some of the functions as @pytest.fixtures, e.g.: https://github.com/nih-sparc/sparc.client/blob/87c7056e7bd5870ed9e758b3038804f1c6afae49/tests/conftest.py#L23C1-L37C31
Basically you define what should be returned by a mocked function and then call them directly from the test, e.g.: https://github.com/nih-sparc/sparc.client/blob/87c7056e7bd5870ed9e758b3038804f1c6afae49/tests/test_pennsieve.py#L40-L47
If there is no workaround, we could create an access token in the repository secrets, but this would be the last resort.
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
70c6b11
) 99.71% compared to head (fe1dad9
) 100.00%. Report is 31 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@athril I checked in the pytests for metadata. The URL handling was also streamlined a bit and an additional check was added to the list and search methods if the user changes the default URL
Hi @athril - any updates on the updates above?
This is done in via a fork of the main branch and with a run of formatters. Discarded formatting changes for files that were not originally changed for the metadata services.