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_hydat to have an option to bypass the keypress question #165

Merged
merged 6 commits into from
Sep 23, 2021

Conversation

rchlumsk
Copy link
Contributor

Small update to download_hydat to add a parameter ask, which will bypass the keypress question and proceed directly to the download of HYDAT if ask=FALSE. Default ask=TRUE makes no change to the existing workflow with the function.

Implemented to have an option for bypassing the keypress when using download_hydat in another function or other situations when the keypress is not ideal.

Copy link
Collaborator

@boshek boshek left a comment

Choose a reason for hiding this comment

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

Looks great! Can you also write a local test that doesn't run on GHA nor CRAN and also run devtools::document. Also a NEWS item would be good. Thanks for this.

R/download.R Outdated Show resolved Hide resolved
R/download.R Outdated Show resolved Hide resolved
R/download.R Show resolved Hide resolved
rchlumsk and others added 4 commits September 23, 2021 10:12
Co-authored-by: Sam Albers <sam.albers@gmail.com>
Co-authored-by: Sam Albers <sam.albers@gmail.com>
Co-authored-by: Sam Albers <sam.albers@gmail.com>
@rchlumsk
Copy link
Contributor Author

Thanks for the suggestions, all make sense! I've added a news item but I am not sure how you would like the test to look or what it would check, could you please more insight on that? I haven't done much with testthat tests myself but happy to give it a shot if you provide a few more parameters. Thanks.

@boshek
Copy link
Collaborator

boshek commented Sep 23, 2021

Actually I don't it is worth the hassle of testing this. Looks really good to me to close #165. Thanks for this.

@boshek boshek merged commit 96c4098 into ropensci:main Sep 23, 2021
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.

2 participants