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

Custom hydat download path fix #203

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Conversation

Travis-Simmons
Copy link
Contributor

Previously if you supplied a 'dl_hydat_here' argument to the 'download_hydat' function it would make it all they way to the end of the function correctly, but then it calls hy_check with no argument. Which does not consider your custom path and causes an error. By supplying the path as an argument it fixes the 'download_hydat' function.

@boshek
Copy link
Collaborator

boshek commented Feb 5, 2024

Thanks for this. The test failures here are unrelated to your PR but I think a test would be good here to have caught the change made in #181. Really this download function needs to be split into multiple small functions (though that's not your problem!).

@boshek
Copy link
Collaborator

boshek commented Feb 9, 2024

@Travis-Simmons is writing a test for this something you'd be able to do to complete this PR?

@Travis-Simmons
Copy link
Contributor Author

@boshek I am willing to write a test for it, but I am unsure how to do that! If you comment some documentation on how to write it, I will give it a go next weekend.

@boshek
Copy link
Collaborator

boshek commented Feb 15, 2024

Actually I don't even really think it needs a test. But can you please write a NEWS entry and include your GitHub handle?

Something like this:

- `download_hydat()` now has an `ask` parameter that can be used to bypass the keypress confirmation when downloading the HYDAT database (@rchlumsk, #165).

@boshek boshek merged commit 1dc75eb into ropensci:main Oct 3, 2024
5 checks passed
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