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

call_MODIS argument cleanup #2519

Closed
infotroph opened this issue Jan 22, 2020 · 1 comment · Fixed by #2606
Closed

call_MODIS argument cleanup #2519

infotroph opened this issue Jan 22, 2020 · 1 comment · Fixed by #2606

Comments

@infotroph
Copy link
Member

I won't hold up the PR for this, but two strong suggestions:

  • Changing the arguments to an exported function is a breaking change, so it should have a CHANGELOG entry. Breaking changes aren't always bad -- I think most users will prefer this version over the old one! -- but when we change interfaces we want to do it cautiously and document the changes conspicuously.

  • When a function has some arguments with default values and some without, it's usually best to put the arguments without default values should be at the beginning of the argument list and the ones with defaults (even if they're NULL) afterwards. Since you're changing argument order in this PR anyway, I encourage doing that now to avoid needing another round of breaking changes to do it later.

Originally posted by @infotroph in https://github.com/_render_node/MDIzOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMjA4Njc2ODEzOnYy/pull_request_review_threads/discussion

Bumped this to a new issue to avoid holding up #2338.

Additional thought: If I'm following right, the outfolder argument is slightly magic in that when it's a path the function both writes to disk and puts the downloaded data in its return value, whereas when outfolder = NULL the function writes no files and only returns the values. This behavior seems reasonable, but should be documented.

@ayushprd
Copy link
Contributor

Hi @infotroph @bailsofhay , I am working on this issue.

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 a pull request may close this issue.

2 participants