-
Notifications
You must be signed in to change notification settings - Fork 38
ADD: Adding ameriflux discovery module. This module is based on the R code amerifluxr #916
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
Conversation
This module is based on the R code amerifluxr
Note, I still need to add tests and examples |
@AdamTheisen Should we leave the default to agree_policy False? So users read the policy first? I was debating how to approach that... |
@zssherman I didn't get a chance to review today but blocking my calendar Wed afternoon |
@AdamTheisen no worries! |
@AdamTheisen I did changes to address your comments. Let me know if the recent commit looks good for all your comments. For the is test I add a kwargs, but once we have the seceret email and username added, ill add one more check to the is test. That way we keep that hidden, but still available for our unit tests |
@AdamTheisen Added the new reader, ready for review when folks have time. Working on the tests still though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zssherman I cc'd you in on the email from the AmeriFlux developers and I think you hit all their requirements. I think this is looking good as well once we get the tests working!
Uh oh!
There was an error while loading. Please reload this page.