-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for HPSS #22
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #22 +/- ##
===========================================
- Coverage 50.88% 47.99% -2.90%
===========================================
Files 16 18 +2
Lines 1476 1642 +166
Branches 313 330 +17
===========================================
+ Hits 751 788 +37
- Misses 669 798 +129
Partials 56 56 ☔ View full report in Codecov by Sentry. |
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.
Looks good.
A few suggestions for follow-up PR:
- in the HSI and HTAR tests, the test for put and get should be separated out, and the same with cvf, xvf and tell. They should be unit tests for the methods they call.
- Some of the arguments that are passed to the HSI and HTAR classes could be improved for user experience. For e.g. if a user wants a verbose mode, the
__init__
class should have averbose = True|False
rather than the user to know the appropriate HSI argument-e
to echo every command. - Use
None
in place of empty strings""
.
I just found a bug in Htar that I am fixing now. I will let you know when it's ready to go again. Should be soon. |
@aerorahul Alright, it's good to go. My tests were only testing the case of sending one file to an HPSS tar archive. Expanding that to test multiple files found that each file being sent to an archive needs its own argument. The test has been updated to check for this and it passes on Hera. |
target = str(target) | ||
source = str(source) |
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.
Is this necessary since the type
is str
as defined in the method signature?
If this could be a Path
, then it makes sense.
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.
The pytests define these inputs as Path
s. The signature is a suggestion -- Python will accept whatever type you throw at it.
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.
looks good.
Description
Adds classes to interface with the hsi and htar commands with several helper methods for easier use. Resolves #2.
Type of change
How Has This Been Tested?
Checklist