-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(filter:date) Timezone formatting issues. #1917
Conversation
@latentflip your build is failing on a test - can you take a look? Thanks! |
Sorry, will do! Pesky browsers! |
Hmm, as best I can tell this is failing because I need to update the tests that are written within ngdoc, which I have done. This is because those previous tests were in fact incorrect. So I have done that, but it seems that there is an Since travis appears to be running that file, which I can't update, I can't get travis to pass, even though my tests are all passing 😦 I am not totally au fait with angular's e2e runner, not testacular, so does anybody know if there is a reason that the above file is checked into testacular itself, that seems broken to me? @vojtajina, any ideas? |
I think the above, although possibly still a question isn't what was tripping me up. Fairly sure I have a passing build now, as soon as Travis reemerges from the dead. |
Alright this is passing now, and I am happy with it. It looks like I was wrong about the angular-scenario stuff above anyway. Anyone got any complaints or is this ready to go? |
holly cow! I'm shocked that we wrote this filter with these two bugs. good job fixing them! simple repro: http://plnkr.co/edit/YbH516RiFCAyFLDp3cJh?p=preview |
reviewed - needs more work, but only simple modifications. otherwise this looks good. |
This commit fixes angular#1261 and angular#1532. This covers two separate issues: - Positive timezones were being formatted without a leading `+` resulting in a formatting string like: "HH:MM:ssZ" giving "12:13:141000" instead of "12:13:14+1000". Fixed by checking if timezone is > 0 and adding a leading "+". - Timezone output signs were inverted. mock.TzDate expects the timezone _offset_ as it's first argument, _not_ the timezone. This means that a mock.TzDate with a positive offset should result in a date string with a negative timezone, and vice-versa.
@IgorMinar thanks for the review! Have updated the commit as suggested and it's still passing. Anything else? |
Bump: @IgorMinar anything else you need me to do on this to get it merged? Thanks. |
PR Checklist (Minor Bugfix)
|
landed as b001c8e thanks! if you haven't received our t-shirt before and would like one please fill out this form: http://goo.gl/075Sj |
Amazing. Thanks @IgorMinar |
This commit fixes #1261 and #1532. This covers two separate issues:
+
resulting in a formatting string like: "HH:MM:ssZ" giving "12:13:141000" instead of "12:13:14+1000". Fixed by checking if timezone is > 0 and adding a leading "+".and vice-versa.
I've tried to stick to the style as much as possible. I have one question about this commit though: ISO_8601 states:
I have chosen that a UTC/0 offset timezone is represented with a Z, like: "12:13:14Z" as that seems to be more normal. Is that okay, or would "12:13:14+0000" be better for some reason?
I have signed the CLA, I am Philip Roberts from Scotland 😄