Enhancement: Make check_resource_dir a general data folder routine#791
Conversation
There was a problem hiding this comment.
Thanks for working on generalizing this method, Rob! I think it's a pretty slick solution you've come up with. I have some small comments and questions on the PR but they should be pretty simple.
I don't believe I've seen any new tests for the check_data_dir or check_feedstock_dir. I think it could be good to add some basic tests on check_data_dir to ensure that new errors are thrown when expected. It may also be good to have one test with check_feedstock_dir just to check that it works before that directory exists in H2I.
Thanks for working on this! If it came in as-is I wouldn't have any concerns. All my comments are pretty small and non-blocking.
Update: can you update the doc page docs/getting_started/environment_variables.md for setting the FEEDSTOCK_DIR as an environment variable (this could also happen in your other PR)
| check_resource_dir = partial(check_data_dir, data_type="resource") | ||
| update_wrapper(check_resource_dir, check_data_dir) | ||
|
|
||
| check_feedstock_dir = partial(check_data_dir, data_type="feedstock") | ||
| update_wrapper(check_feedstock_dir, check_data_dir) |
There was a problem hiding this comment.
are these lines supposed to be here?
There was a problem hiding this comment.
what do they do? I've never seen partial or update_wrapper used. Could you add some in-line comments?
There was a problem hiding this comment.
partial functions create wrapper functions with the args/kwargs partially passed. It's a convenient way to pre-fill function calls with common parameterizations. For instance, check_resource_dir is now just a wrapper equal to check_data_dir(data_type="resource"), enabling the resource classes to use basically the same folder checking function calls as before this PR.
update_wrapper simply applies the docstring from one function to another, and is primarily for partial functions so that they can also have documentation.
There was a problem hiding this comment.
that is very slick! Thanks for explaining that!
…com/RHammond2/H2Integrate into feature/resourece-dir-to-file-utils
…com/RHammond2/H2Integrate into feature/resourece-dir-to-file-utils
elenya-grant
left a comment
There was a problem hiding this comment.
This looks great Rob! Thanks for working through the annoying debugging process and making this method generalizable! I think this looks awesome!
… caused headaches
…com/RHammond2/H2Integrate into feature/resourece-dir-to-file-utils
johnjasa
left a comment
There was a problem hiding this comment.
Thanks for this, @RHammond2 and @elenya-grant, and I'm sorry it was such a bear to debug! Really glad you got it working so well.
Enhancement: Make
check_resource_dira general data folder routineThis PR is in service of #777 to enable the usage of the functionality in
h2integrate/resource/utilities/file_tools.py::check_resource_dirmore broadly applicable to other data types (e.g. feedstocks). By creating the generalcheck_data_dirwith a thin wrapper forcheck_resource_dirthat populates thedata_typearg with "resource" much of the same usage is maintained through the resource data classes. Similarly the existing tests are able to be used with comparable test coverage and minimal changed code.Section 1: Type of Contribution
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.md"A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
XYZshould be replaced with the actual number.Section 4: Related Issues
N/A
Section 5: Impacted Areas of the Software
Section 5.1: New Files
N/A
Section 5.2: Modified Files
h2integrate/resource/utilities/file_tools.py: removed in favor of movingcheck_resource_dirto the below modified file.h2integrate/core/utilities/file_utils.py:check_data_dir: Generaldata_typechecking data directory checking with default handling, exactly as prescribed from the previouscheck_resource_dir, except with the flexibility of working with multipledata_type, e.g. "resource", "feedstock", or whatever types of data might be used later.check_resource_dir:partialfunction ofcheck_data_dirwithdata_type="resource"pre-populated.check_feedstock_dir: Same as above, but for "feedstock".h2integrate/core/test/test_utilities.pytemp_resource_dir_env: New fixture to ensure the environment variable is set and unset regardless of a test failure.test_check_resource_dir_environment_var: uses a fixture to modify theRESOURCE_DIRenvironment variable.test/conftest.py:pytest_sessionstart: Stores the originalRESOURCE_DIRvalue in a temporary variable.pytest_sessionfinish: Restores the original `RESOURCE_DIR value from the temporary variable.Section 6: Additional Supporting Information
This PR stems from a tangential issue in #777 where there is a need to reuse existing functionality, but is hard-coded for a specific use case making this work worthy of its own PR.
Section 7: Test Results, if applicable