-
Notifications
You must be signed in to change notification settings - Fork 182
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
Split DateTimeFormat keys #774
Conversation
@sffc does it look generally like the right direction to you? How should I handle |
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.
@sffc does it look generally like the right direction to you?
Yes, this looks like what I had in mind!
How should I handle StructProvider in such a composed bound scenario?
I would think that in order to make a StructProvider return multiple types of structs, one would have two StructProvider instances and then tie them together with another class that delegates to one or the other. Perhaps we could make a helper macro for that.
95aaae9
to
4fc5982
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Codecov Report
@@ Coverage Diff @@
## main #774 +/- ##
==========================================
- Coverage 76.16% 75.52% -0.64%
==========================================
Files 189 192 +3
Lines 12153 12296 +143
==========================================
+ Hits 9256 9287 +31
- Misses 2897 3009 +112
Continue to review full report at Codecov.
|
This is now ready for review. It seems to give a 5-8% win in |
Pull Request Test Coverage Report for Build 39aed657b0376692d800deb921232988975fa000-PR-774
💛 - Coveralls |
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!
4fc5982
to
d1d8410
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
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!
Fixes #257