Skip to content
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

DateTime support in DateFormatter. #476 #479

Merged
merged 2 commits into from
Nov 20, 2017
Merged

Conversation

lemonboston
Copy link
Contributor

@dmfs Could you please review the conversion and Time-DateTime equivalence check in the test extra thoroughly?
You know the domain better, to assess that this conversion is good for the project or not.

@dmfs
Copy link
Owner

dmfs commented Nov 17, 2017

your tests don't seem to validate the actual Time values, i.e. minute, day', etc. Also I think your all-day handling might be incorrect. When an event is allday the timezone should be set to UTC. I'm not exactly sure what DateTimereturns in that case, maybe it already returnsUTC`.

@lemonboston lemonboston force-pushed the mx/476-datetime-formatter branch from 3965e9f to 3003fb9 Compare November 17, 2017 17:00
@lemonboston
Copy link
Contributor Author

I've added checking the individual fields in the test.

Also updated conversion to use "UTC" for all-day. But please note/check that Time constructors are used in different ways in the project, so if I searched for them, both the paremterless and with the timezone, so this "UTC" as default didn't look totally consistent from the first look. But I haven't checked the details.

Also rebased on master.

Ready for check again.

Copy link
Owner

@dmfs dmfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@dmfs
Copy link
Owner

dmfs commented Nov 20, 2017

Merging this despite the CI issue.

@dmfs dmfs merged commit 9b0f528 into master Nov 20, 2017
@dmfs dmfs deleted the mx/476-datetime-formatter branch November 20, 2017 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants