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

download_zenodo(): more features #69

Merged
merged 19 commits into from
Feb 26, 2020
Merged

download_zenodo(): more features #69

merged 19 commits into from
Feb 26, 2020

Conversation

florisvdh
Copy link
Member

New message is now: Will download 11 files (total size: 1.4 GiB) from DOI: .........

Copy link
Contributor

@ThierryO ThierryO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How strong are the benefits of this new feature compared to the cost of having yet another dependency?

@florisvdh
Copy link
Member Author

The benefit of this information is not neglible for larger datasets.

An alternative could be to copy over the function code of humanize into inborutils, but that seems not so elegant. The humanize package itself is small. Maybe there are alternative packages.

@florisvdh
Copy link
Member Author

@ThierryO , FWIW I've just been looking for alternative functions (present on my own installation :-)). Found no 'common package' dependency for this, though - even units has no function to automatically return human-readable output. Is depending on R.utils package more convenient?

> R.utils::hsize(78945)
[1] "77.1 KiB"
> gdata::humanReadable(78945)
[1] "77.1 KiB"
> humanize::natural_size(78945, suffix_type="binary")
77.1 KiB
> prettyunits::pretty_bytes(78945)
[1] "78.94 kB"
> fs::fs_bytes(78945)
77.1K> 

Only the first three examples are able to present the binary (as opposed to decimal) representation (which I believe is the common standard to be taken) as a character.

@florisvdh
Copy link
Member Author

florisvdh commented Jan 29, 2020

To make the latter ('character') clear for the less obvious cases:

> class(humanize::natural_size(78945))
[1] "glue"      "character"
> class(fs::fs_bytes(78945))
[1] "fs_bytes" "numeric" 

Also, it can be seen that fs::fs_bytes() does not use the IEC symbol of the kibibyte.

@florisvdh florisvdh changed the title download_zenodo(): reveal total file size to user download_zenodo(): more features Feb 20, 2020
@florisvdh
Copy link
Member Author

@hansvancalster @ThierryO modifications and extras:

  • dropped humanize dependency; now uses internal function human_filesize()
  • tweaked messages
  • added argument quiet to suppress messages (default: FALSE)

Copy link
Contributor

@hansvancalster hansvancalster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me, just one extra assert_that needed.
Only looked at the code, not tested.

R/download_zenodo.R Outdated Show resolved Hide resolved
Co-Authored-By: Hans Van Calster <hans.vancalster@inbo.be>
Copy link
Contributor

@hansvancalster hansvancalster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me, but will await @ThierryO review before merging.

Copy link
Contributor

@ThierryO ThierryO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. Just a few minor comments.

R/download_zenodo.R Outdated Show resolved Hide resolved
R/download_zenodo.R Outdated Show resolved Hide resolved
R/download_zenodo.R Show resolved Hide resolved
Co-Authored-By: Thierry Onkelinx <ThierryO@users.noreply.github.com>
@florisvdh
Copy link
Member Author

Thanks @ThierryO for the extra suggestions. Will look at remaining topics next week!

@florisvdh florisvdh merged commit a4d2b5d into master Feb 26, 2020
@florisvdh florisvdh deleted the download_zenodo_size branch February 26, 2020 10:09
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.

3 participants