-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Hide repeats in datetimeformatter #13777
Hide repeats in datetimeformatter #13777
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-3.5 #13777 +/- ##
==============================================
+ Coverage 91.57% 92.17% +0.59%
==============================================
Files 326 326
Lines 20737 20763 +26
==============================================
+ Hits 18990 19138 +148
+ Misses 1747 1625 -122 |
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.
A few initial comments @muendlein but I'm also not 100% clear on this change. Please add some before/after screenshots to aid quick comparison
@bryevdv now with some screenshots
|
@muendlein OK now the code makes more sense, but now I am confused by the help string:
This seems to be exactly opposite of what this does, since it specifically hides the context portion of the ticks in your screenshots above. Perhaps you mean something like "Context configuration is unaffected by this setting" but I don't think that's enough information to let users predict what this setting will do. We might also want to consider making the name more descriptive in some way cc @tcmetzger |
@bryevdv I have added a bit more details to the help string in order to make it less confusing. Note: In my example above the hiding repeats is activated for the context directly and not the parent formatter. |
@muendlein can you merge/rebase latest and we can see if the windows errors go away? |
In considering the help string above I realized what I don't love about this approach. Ideally, we would hide ticks we don't want to see by removing the tick locations themselves altogether. "Removing" them by just replacing them with the empty strings seems like a bit of a kludge. But we don't pathways for that kind of feedback between the formatter and the ticker, and it's probably not worth trying to implement them, so I this approach will have to do. |
b81a0c9
to
eaf6838
Compare
@bryevdv There seems to be an issue with the CI build. |
@muendlein Yes it seems to be affecting all PRs at the moment. I'll get this merged once it is sorted. edit: for reference |
@muendlein the broken setuptools releases have been yanked, hopefully just restarting CI will work now 🤞 |
Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>
Thanks @muendlein ! |
All pull requests must have an associated issue in the issue tracker. If there
isn't one, please go open an issue describing the defect, deficiency or desired
feature. You can read more about our issue and PR processes in the
wiki.