search

sailrish / shipit

shipit node module
MIT License
86 stars 65 forks source link

Remove fed ex iso #28

Closed cmswalker closed 8 years ago

cmswalker commented 8 years ago

per issue https://github.com/sailrish/shipit/issues/27

sailrish commented 8 years ago

@cmswalker and @katylava: sorry for the delay in responding. I think this change looks good. However, I'll need to think a bit more about how to keep this consistent across all shipping carriers, including those that don't provide a timezone in their timestamps. Perhaps the best course of action in such cases is to assume the timezone is UTC. But then we might need another attribute for a shipping carrier that indicates whether a timezone was detected, or was it implicit and forced to UTC. Once I get a chance to think about that, I'll merge this PR.

cmswalker commented 8 years ago

@sailrish sounds good, thanks

sailrish commented 8 years ago

Hey folks, So I thought about this a bit more, and I like your implementation. I have opened a new PR #29 which is slightly different from yours. The timestamp attribute of an activity is just like in your PR #28, but I have added a new datetime attribute which reflects the local time for the location in question. Just so that clients of this node module that are expecting a local time don't break. Otherwise, they would have to invoke something like Google Map's timezone API to determine the local time. Finally, I have also updated the A1 International implementation which had a similar bug.

sailrish commented 8 years ago

Closing this PR (replaced by #29)