Fix ambiguous time issue

twitter / twitter-cldr-rb

Ruby implementation of the ICU (International Components for Unicode) that uses the Common Locale Data Repository to format dates, plurals, and more.

Apache License 2.0

672 stars 93 forks source link

Fix ambiguous time issue #243

Closed DRBragg closed 3 years ago

DRBragg commented 3 years ago

We ran into an odd issue where passing certain UTC times to display_name_for would raise an TZInfo::AmbiguousTime exception. A little digging lead me to to this issue in the tzinfo repo. This PR addresses this issue by passing a "fallback" block to period_for_local.

I wasn't sure the preferred way to add tests so I added a simple "demo" one. I'm happy to update/add more if desired.

CLAassistant commented 3 years ago

All committers have signed the CLA.

camertron commented 3 years ago

Hey @DRBragg, thanks for the PR!

This is so funny, I'm working on a blog post right now that explores this exact issue in-depth.

I don't think it's possible to know which period a particular caller will want. From what I can tell from looking at other apps/libraries, it's common practice to pass the block through to tzinfo instead of defaulting to &:first or &:last. In other words, I think we should do something like this:

def period_for_local(date, &block)
  tz.period_for_local(date, &block)
end

or even better:

def period_for_local(*args, &block)
  tz.period_for_local(*args, &block)
end

Then it's up to callers to provide a block and choose the time zone period they want.

DRBragg commented 3 years ago

Thanks for looking at this so quickly @camertron!

I orginally was trying to limit the scope of this PR, but I agree that it would be better to pass the block through. I'm 100% cool with making the change you suggested but, since the actual issue I'm trying to solve is with display_name_for, it will come with some more changes. Do you think it would be best to allow display_name_for to also accept a block or to default to :first in there since we don't actually care about the "time"?

DRBragg commented 3 years ago

Hey @camertron, sorry for the extra commits. A colleague of mine pointed out that always passing :first as a block to period_for_local would result in always being in dst. I did a little more digging and it looks like period_for_utc might be a better solution for this issue. I updated the helper and improved the tests. Right now I'm calling period_for_utc on orig_tz since a TwitterCldr instance doesn't have a delegating method but I'd be happy to add one if you'd like. Let me know what you think. Thanks!

DRBragg commented 3 years ago

Sorry for being a PITA @camertron turns out non-UTC times can be ambiguous too.

DRBragg commented 3 years ago

@camertron I made the change you requested. Now display_name_for takes (and passes down) all the same arguments as period_for_local. I updated the specs to better illustrate this but writing specs is one of my weaker areas so I'm happy to add/adjust/remove any of them if you'd like. If you're satisfied with this PR I'll update the README to reflect the changes.

camertron commented 3 years ago

Thanks for making that change @DRBragg :)

Can you explain a bit about why the dst flag is necessary? Isn't DST information handled by the time zone data? Or is the idea to pass all the flags period_for_local accepts?

DRBragg commented 3 years ago

Thanks for making that change @DRBragg :)

Can you explain a bit about why the dst flag is necessary? Isn't DST information handled by the time zone data? Or is the idea to pass all the flags period_for_local accepts?

Yea, just passing through all the same args that period_for_local accepts.

camertron commented 3 years ago

Published in v6.9.0 :)