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

EtOrbi.parse unable to parse some timezones that Time.parse can #30

Closed
jsmartt opened this issue Feb 19, 2021 · 8 comments
Closed

EtOrbi.parse unable to parse some timezones that Time.parse can #30

jsmartt opened this issue Feb 19, 2021 · 8 comments
Assignees

Comments

@jsmartt
Copy link

jsmartt commented Feb 19, 2021

I'm trying to parse user-given times for scheduled events, and there are certain timezones parsable by Time and DateTime that are ignored by EtOrbi.

EtOrbi.parse('7AM PST').to_s    # "2021-02-19 07:00:00 +0000"
DateTime.parse('7AM PST').to_s  # "2021-02-19 07:00:00 -0800"
Time.parse('7AM PST').to_s      # "2021-02-19 07:00:00 -0800"

EtOrbi.parse('7AM MDT').to_s    # "2021-02-19 07:00:00 +0000"
Time.parse('7AM MDT').to_s      # "2021-02-19 07:00:00 -0600"

I've done a bit of research and have a suggestion to offer. Perhaps EtOrbi.extract_zone can use Date._parse to determine if the string given has timezone info provided, and if so, use that:

def extract_zone(str)

  s = str.dup

  d = Date._parse(str)
  if d && d[:offset]
    offset = "UTC#{d[:offset] / 3600}"
    s.gsub!(d[:zone], offset) if ZONES_OLSON.include?(offset)
  end
  
  # Rest of original code from method
end

This allows the ZoneOffsets parsable by Time.parse to be recognized here too.

I recognize that there are a small number of timezones affected here, mainly US and military timezones, so maybe it's not worth it? I'd definitely like to hear a maintainer's thoughts.

Thanks!

@jsmartt jsmartt changed the title EtOrbi.parse() ignores timezone EtOrbi.parse unable to parse some timezones that Time.parse can Feb 19, 2021
@jmettraux jmettraux self-assigned this Feb 20, 2021
@jmettraux
Copy link
Member

Hello,

@jsmartt wrote:

if there is anything et-orbi can do to persist user-provided timezone info?

I would prefer et-orbi not storing anything, just making the best possible decision given context and input.

I will look at your proposition, it looks straightforward.

Give me a bit of time, thanks for the feedback!

@jmettraux
Copy link
Member

@jsmartt

By the way, in which context are you using et-orbi? Via fugit or via rufus-scheduler?

@jmettraux
Copy link
Member

OK, it seems that the promise of Ruby dealing with timezones is here for good, gh-16

@jsmartt
Copy link
Author

jsmartt commented Feb 23, 2021

Thanks for looking into it @jmettraux. I'm using etorbi in the context of fugit. And I've updated the description; I left in a blurb about tzinfo and timezones that was from a previous draft, and is irrelevant; sorry for the confusion.

@jsmartt
Copy link
Author

jsmartt commented Feb 23, 2021

I'm actually just now realizing some other behavior that makes this suggested change a bit useless for me. It doesn't look like EtOrbi.get_tzone knows how to handle some of the special timezones supported by EtOrbi.extract_zone, and visa-versa. This causes failures in EtOrbi.parse. For example:

EtOrbi.extract_zone('7AM -07:00') # ["7AM -07:00", nil]
EtOrbi.get_tzone('-07:00')        # <TZInfo::DataTimezone: -7:00>
EtOrbi.parse('7AM -07:00')        # <EtOrbi::EoTime:0x00007f7c172c96c0 @seconds=1613977200.0, @time=nil, @zone=#<TZInfo::DataTimezone: Etc/UTC>>

EtOrbi.extract_zone('7AM UTC-7') # ["7AM", "UTC-7"]
EtOrbi.get_tzone('UTC-7')        # Error
EtOrbi.parse('7AM UTC-7')        # Error

EtOrbi.extract_zone('7AM GMT-7') # ["7AM -7", "GMT"]
EtOrbi.get_tzone('GMT-7')        # <TZInfo::DataTimezone: GMT>
EtOrbi.parse('7AM GMT-7')        # <EtOrbi::EoTime:0x00007f7c21f00f10 @seconds=1614063600.0, @time=nil, @zone=#<TZInfo::DataTimezone: GMT>>

# These all work as expected:
EtOrbi.extract_zone('7AM Etc/GMT-7') # ["7AM", "Etc/GMT-7"]
EtOrbi.get_tzone('Etc/GMT-7')        # <TZInfo::DataTimezone: Etc/GMT-7>
EtOrbi.parse('7AM Etc/GMT-7')        # <EtOrbi::EoTime:0x00007f7c2020aa28 @seconds=1614038400.0, @time=nil, @zone=#<TZInfo::DataTimezone: Etc/GMT-7>>

There is quite a mismatch in terms of which zones are supported by these different methods. Is that expected? It would be nice if the zones like UTC-8, GMT-8, & -08:00 were fully supported across the board if they are in 1 place.

@jmettraux
Copy link
Member

jmettraux commented Feb 23, 2021

Hello,

actually, what I am expecting for fugit is proper TZ database names like "Africa/Abidjan". TZ abbreviations are ambiguous and then there are instances like CET (Central European Time) vs CEST (Central European Summer Time).

TZ database names are less ambiguous and with proper tzinfo management, they are more resilient to daylight saving policies and other political changes.

I will still look at your examples and let et-orbi and fugit do their best.

I will also explain TZ abbreviation vs TZ database names in fugit's readme.

jmettraux added a commit that referenced this issue Feb 23, 2021
@jsmartt
Copy link
Author

jsmartt commented Feb 23, 2021

Ok. The TZ database names definitely seem like the best option for now, so I'll make sure my users know to use those. 👍
Users are inputing these time strings, which is why I wanted etorbi/fugit to be able to parse a variety of additional formats, but I totally understand that not all arbitrary formats can or should be supported. Thanks for the clarification on all this!

@jmettraux
Copy link
Member

Since you probably stand between your user input and passing it to fugit and friends, you could translate "PST" and "PDT" to "America/Los_Angeles" before passing it. You know your users timezones better than fugit and et-orbi do.

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

No branches or pull requests

2 participants