-
Notifications
You must be signed in to change notification settings - Fork 443
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
Bugfix - Explicit Time Zone assignment regression from #532 #539
Bugfix - Explicit Time Zone assignment regression from #532 #539
Conversation
wasn't passing in the timezone. Fixing...
a common constant for later reference.
prone reusability and easier cross-class documentation
objects with different TZs
Took a few tries to catch all the places where we were "doing it wrong", but it's good now. Thank god for tests... /cc: @koenpunt @jpfuentes2 |
* | ||
* @var string | ||
*/ | ||
const DATETIME_TRANSLATE_FORMAT = 'Y-m-d\TH:i:s'; |
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.
Why the extra T
Here?
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.
It's just the RFC 3339 format without the timezone identifier. I figured it was the most easily (standardized, spec-based) identifiable format. It doesn't really matter, though, as it's just for internal translations (not for storage, etc).
Thanks @jpfuentes2. Tests FTW. |
So PR #533 was fantastic in fixing an issue regarding passing in an explicit time-zone during column casting and attribute assigning. Unfortunately, the merge of #532 caused a regression as it overwrote the fix made in #533. 😞
This PR fixes that regression.
The included test makes sure the behavior is correct, and the following snippet demonstrates the issue at hand: