-
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
Add DateTimeFormat #202
Add DateTimeFormat #202
Conversation
b5efd3a
to
7613711
Compare
027dd8f
to
ad5f829
Compare
I applied a generic feedback I collected on the WIP over the last week - in particular, I took Shane's comment about "maybe a string of skeleton/pattern is not the fastest way" and run away with it. I now have fields that Skeleton and Pattern can use (I don't have a Pattern yet, but basically an iterator over Two things I'd like to have a vision for next are:
@mihnita and @sffc - can I get your help with this? |
92405c8
to
0707300
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
df29aa7
to
9d59a0b
Compare
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.
Thank you for applying my suggestions!
We'll need to think more about the form of the Year/Month/Day types when we introduce non-Gregorian calendar systems in a future PR that hopefully lands soon after this one.
day: Day, | ||
hour: Hour, | ||
minute: Minute, | ||
second: Second, |
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.
Nit: I think you added millisecond at some point, but now it's gone?
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.
Yes, sorry for not explanining. I initially prototyped a larger version of this api to understand the facets that included most of ldml, skeletons, components etc. Then, in result of our agreement to trim it to minimum I removed almost all pieces that weren't strictly needed and yet the review took over 150 comments. Shane called out a number of places where I bits and pieces were left that aren't needed for this PR and I went through a cleanse to remove them all. We don't use milliseconds in styles so I removed it and will bring back when we work on skeletons.
Is that ok with you?
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.
(setting review bit)
This is a port of https://github.com/zbraniecki/unic-datetime for ICU4X.
The intention of this PR is to get to the point where we have the first formatter API MVP ready to land.
The MVP should contain:
DateTimeFormat
constructor andformat
FormattedDateTime
Write
and toString
The integration into DataProvider is intended to be handled in a separate PR.