-
Notifications
You must be signed in to change notification settings - Fork 11
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 option to specify tars to check or extract #170
Conversation
c8164ba
to
baffaaa
Compare
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.
@golaz This is ready for review.
zstash/extract.py
Outdated
else: # TODO: should this always be run (i.e., not if/else?) | ||
# Find matching files | ||
for args_file in args.files: | ||
cur.execute( | ||
u"select * from files where name GLOB ? or tar GLOB ?", | ||
(args_file, args_file), | ||
) | ||
matches_ = matches_ + cur.fetchall() |
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.
Should we still run this section if --tars
is being used? That is, is there a use case where a user would specify zstash check --hpss=<HPSS> --tars=<tars> <specific files>
? It seems like if a user is requesting specific files, it wouldn't be necessary to specify which tars to look at.
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.
That's right. When specifying the tar files, we don't need the --tars
option - It's redundant.
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.
@tangq If a user specifies both <tars>
and <specific files>
, what should be the expected behavior?
- Just check/extract
<tars>
- Just check/extract
<specific files>
- Print error message saying to only specify one
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.
I'd say option 3. What do you think?
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.
I would agree. @golaz which is your preference?
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.
@forsyth2 and @tangq : this is an edge case and I would think that it is unlikely a user would want to specify <tars>
and <specific files>
simultaneously. Therefore, if it's relatively straightforward to implement, I would suggest option 3 as well to make sure there is no room for misunderstanding from an unsuspecting user.
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 by visual check. If testing successfully and removing the redundant --tars
option when specifying the file names to check, it can be merged.
zstash/extract.py
Outdated
else: # TODO: should this always be run (i.e., not if/else?) | ||
# Find matching files | ||
for args_file in args.files: | ||
cur.execute( | ||
u"select * from files where name GLOB ? or tar GLOB ?", | ||
(args_file, args_file), | ||
) | ||
matches_ = matches_ + cur.fetchall() |
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.
That's right. When specifying the tar files, we don't need the --tars
option - It's redundant.
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.
@forsyth2 : the code looks good. I made a few minor suggestions. Let me know if you'd like me to try it out myself.
de9857b
to
e139de7
Compare
Thanks @forsyth2. The changes look good to me. |
e139de7
to
1f8dbe3
Compare
Specify starting tar. Resolves #164.