Repository navigation
BUG:datetime list starting with none - #23587
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
MikiPWata
changed the title
MikiPWata
changed the title
638af82 to
8276ebf
Compare
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
This should be squashed merged or squashed and rebased.
|
@MikiPWata Do you feel comfortable squashing this to one commit? |
yes! I will squash them into 1 commit |
68a3122 to
997890d
Compare
|
Thank you, this is now waiting for a second reviewer to merge. |
|
Please squash to one commit again. Doing this as return next(val for val in obj if not (skip_none and val is None)) # I _think_ this logic is correctmight be a bit more elegant. |
08ec45a to
15d5021
Compare
|
not sure why many pytests are failing.... |
|
Test failures are likely unrelated https://gitter.im/matplotlib/matplotlib?at=62f2d60b9d3c186299235d4e |
038c51f to
2328501
Compare
| return next(iter(obj)) | ||
|
|
||
|
|
||
| def _safe_first_non_none(obj, skip_none=False): |
There was a problem hiding this comment.
| def _safe_first_non_none(obj, skip_none=False): | |
| def _safe_first_non_none(obj, skip_none=True): |
There was a problem hiding this comment.
Only from mobile, so maybe I’m missing some context, but why do we have two methods now? Can’t we integrate the logic easily in the original method, that now grew the skip_none parameter already? Also, having a method safe_first_non_none() that is configurable via skip_none seems contradictory in itself.
There was a problem hiding this comment.
@timhoffm
I created the _safe_first_non_none() as a helper function. I guess we need to use args/kwargs as well if we are integrating the logic in safe_first_element()?
|
|
||
|
|
||
| def safe_first_element(obj): | ||
| def safe_first_element(obj, skip_none=False): |
There was a problem hiding this comment.
| def safe_first_element(obj, skip_none=False): | |
| def safe_first_element(obj): |
|
And with the suggested changes we need to find all our usage of This way we (internally) always use the new behavior, if anyone is using this method for their own reasons they keep the same old behavior, and we only have one copy of the logic! |
|
I think these test failures are real and caused by the changes in this PR. |
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
102a145 to
a8c01a4
Compare
I have also changed all |
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
We should also consider deprecating safe_get_first_element, but I think we have asked @MikiPWata to do enough!
|
Thanks @MikiPWata! |
PR Summary
#23580
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).