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

Update download_vpfiles() and select_vpfiles() #176

Merged
merged 13 commits into from Dec 13, 2018
Merged

Conversation

peterdesmet
Copy link
Collaborator

For both functions

download_vpfiles()

Updated syntax:

download_vpfiles(
  date_min = "2016-10-03", # required
  date_max = "2016-10-05", # required
  radars = c("bejab", "bewid"), # required
  directory = "my_data" # optional, default: "."
  overwrite = TRUE # optional, default: FALSE
)
  • Unzip to 5 letter radar code directory. Fixes Don't unzip files with download_vpfiles() #172 (comment) 👌

  • Add new optional parameter overwrite = FALSE (default) to skip downloading previously downloaded files 🎉

  • Just use curl, not httr. Fixes Use httr instead of (r)curl #150 👌

  • Simplify path, url, file name creation in function

  • Nicer messaging of download status, using message() not print() so they can be ignored in Rmd 🎉

    bewid201710.zip: already downloaded
    bejab201711.zip: http error 404
    bewid201711.zip: successfully downloaded
    

select_vpfiles()

Updated syntax:

select_vpfiles(
  date_min = "2016-10-03", # optional
  date_max = "2016-10-05", # optional
  radars = c("bejab", "bewid"), # optional
  directory = "my_data" # optional, default "." and moved to last position to align with download_vpfiles()
)

- Remove check_url_existence() which relied on RCurl. Instead delete the erronously created zip file on http error instead. @stijnvanhoey I think this might also improve the speed of the function, as before the file was downloaded twice: with getBinaryURL() and curl_download()
- Add some explanation to example
- Use warning() not print() to show files being downloaded
- Use silent download (no progress shown)
- Warn with status of http, e.g.

```
Downloading https://lw-enram.s3-eu-west-1.amazonaws.com/be/jab/2016/bejab201610.zip
Downloading https://lw-enram.s3-eu-west-1.amazonaws.com/be/wid/2016/bewid201610.zip
Downloading https://lw-enram.s3-eu-west-1.amazonaws.com/be/jab/2016/bejab201611.zip
HTTP error 404.
Downloading https://lw-enram.s3-eu-west-1.amazonaws.com/be/wid/2016/bewid201611.zip
HTTP error 404.
```
Rewrite function:

- Drop country parameter #173
- Use and check 5 letter radar codes
- Add overwrite parameter
- Simplify file_name, file_path, url creation
- Simplify download directory to countryradar: #172 (comment)
- Drop lubridate dependency
- Use curl_fetch_disk()
- Write nice output messages:

```
bewid201710.zip: already downloaded
bejab201711.zip: http error 404
bewid201711.zip: successfully downloaded
```
Address #173

- Use 5 letter radar code, drop country
- Move directory parameter to end (cf. download_vpfiles)
- Check if radar codes are 5 letters
- Drop lubridate dependency
- Make dates optional
- Update documentation
R/download_vpfiles.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@stijnvanhoey stijnvanhoey left a comment

Choose a reason for hiding this comment

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

Nice work. As we will have to do it anyway, please provide unit tests directly when refactoring/adding funcitons. If you need any help writing the tests, just let me know.

R/download_vpfiles.R Show resolved Hide resolved
R/download_vpfiles.R Show resolved Hide resolved
R/select_vpfiles.R Show resolved Hide resolved
R/select_vpfiles.R Outdated Show resolved Hide resolved
R/select_vpfiles.R Show resolved Hide resolved
R/select_vpfiles.R Show resolved Hide resolved
@peterdesmet
Copy link
Collaborator Author

peterdesmet commented Dec 10, 2018

As we will have to do it anyway, please provide unit tests directly when refactoring/adding funcitons. If you need any help writing the tests, just let me know.

I agree completely, but have never done this so I would need a crash course from you. 😁 To cover:

@adokter
Copy link
Owner

adokter commented Dec 10, 2018

I agree completely, but have never done this so I would need a crash course from you. 😁

Same here!

@adokter adokter mentioned this pull request Dec 11, 2018
3 tasks
@peterdesmet peterdesmet mentioned this pull request Dec 13, 2018
2 tasks
@stijnvanhoey
Copy link
Collaborator

Nice work @peterdesmet

@peterdesmet peterdesmet merged commit 8a615f4 into develop Dec 13, 2018
@peterdesmet peterdesmet deleted the download branch December 13, 2018 15:16
@peterdesmet peterdesmet added this to the 0.4.0 milestone Dec 13, 2018
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