-
Notifications
You must be signed in to change notification settings - Fork 303
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
Fix Scene modifying user-provided reader_kwargs #2335
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2335 +/- ##
=======================================
Coverage 94.58% 94.59%
=======================================
Files 314 314
Lines 47518 47526 +8
=======================================
+ Hits 44943 44955 +12
+ Misses 2575 2571 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Codecov seems to think the fix isn't covered by the modified tests.
I don't know exactly what codecov's problem is. Everything is tested now and when you click on the codecov Details it shows that things are covered...I think. As discussed on slack with @pnuu, this PR now also clarifies/fixes the case where storage_options are defined at the "global" reader_kwargs level and the per-reader level (use global as defaults, overwrite with reader-specific). |
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.
LGTM
While working with the
MultiScene
I noticed that only the first Scene creation was working and failing on the second becausereader_kwargs
passed by me were being modified by the Scene. This is the simplest fix I could think of for fixing this.