-
Notifications
You must be signed in to change notification settings - Fork 998
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
Add kwargs to forecast.get_data with tests #746
Conversation
So tests are passing, but the codecov failed because these lines weren't hit. I copied them from elsewhere in the file. What is your policy on codecov needing to pass for tests? |
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.
Looks good, just a few minor things below...
I don't understand why the diff coverage test fails. I think it's ok to ignore it.
Ah, I missed your comment when finishing my review. This PR is about fixing an API issue. I don't think it's important that it tests details of the returned data. |
Thanks @odow! |
pvlib python pull request guidelines
forecast.get_data
doesn't allow extra kwargs #745Running locally, I get some warnings. Should I fix these? Also, the tests take 60 seconds on my machine which seems a bit long for this one file. Shall I merge some tests? Or any other ideas for speeding it up?
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.