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

Irwin download script #15

Merged
merged 2 commits into from
Oct 21, 2014
Merged

Irwin download script #15

merged 2 commits into from
Oct 21, 2014

Conversation

iljungr
Copy link
Contributor

@iljungr iljungr commented Oct 21, 2014

Daniel,
As I was creating tools for the various remaining commands in taxon_filter, I realized it would be nice to have a subclass of DownloadPackage that downloads stuff that doesn't require any installation except possibly unpacking. In implementing this DownloadScript class, I found some of the behaviors of DownloadPackage to be inconvenient. I changed them, but want to run the changes by you.

  • I want the script to end up in the same place whether it is zipped or not, so I added a line to unpack that copies from the download_dir to the unpack_dir for files that aren't zipped or tarred.
  • I renamed unpack_dir to destination_dir since it is no longer always unpacked there.
  • Given this behavior, I couldn't think of any reason anyone would want a download_dir other than tempfile.gettempdir(), so I removed that argument from DownloadPackage and always use the tempdir.
  • I moved the call to unpack out of post_download and put it into download.
  • Unrelated to the above changes, I renamed requireExecutability to require_executability to be more consistent with your argument naming.

@dpark01
Copy link
Member

dpark01 commented Oct 21, 2014

That's about 6 lines of code (one extra subclass) and seems useful, sounds great!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a0c97a2 on irwin-DownloadScript into 9b17156 on master.

dpark01 added a commit that referenced this pull request Oct 21, 2014
add DownloadScript, subclasses DownloadPackage
@dpark01 dpark01 merged commit c4cb657 into master Oct 21, 2014
@dpark01 dpark01 deleted the irwin-DownloadScript branch October 21, 2014 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants