Rails Ruby on Rails → 2.1.1

Comments and changes to this ticket

• You flagged this item as spam.
  

  Star Trader May 14th, 2008 @ 10:43 PM

  I agree with the stated purpose of the ticket, but I'm dubious of the patch. The regular expressions given in the convince methods seem to return more results then expected. E.g. #pacific_zones would include the the U.S. Pacific time zone but not the Alaskan.

  Before this patch is applied, I think further discussion need to happen about just which timezones to group together. This should be based on potential user bases, and not necessarily on broad regular expressions

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 10:24 AM

  Hello Star Trader,

  thank you for the review and valuable input. I do agree that further discussion as to which time zones shall be grouped together is necessary.

  The patch I sent in is merely a quick and dirty hack that provides convenience methods for regions outside the U.S., and naturally, it can be enhanced.

  Having said that, I would like to suggest that you might have another look at the code with reference to you claim that the U.S. pacific time zone would be in pacific_zones whereas the Alaskan time zone is not.

  This is not true. Both the U.S. pacific time zone and the Alaskan time zone are in american_zones but not in pacific_zones.

  The regular expression I use in the patch does not match against the Ruby on Rails time zone names but against the TZinfo names, f.e. "Europe/Dublin" or "Pacific/Samoa".

  Since there is no TZinfo with "Pacific/Alaska" (or similar), the Alaskan time zone is not included in pacific_zones. The same holds for the other convenience methods.

  The following time zones are in pacific_zones:

  • International Date Line West
  • Midway Island
  • Samoa
  • Hawaii
  • Guam
  • Port Moresby
  • New Caledonia
  • Auckland
  • Fiji
  • Marshall Is.
  • Wellington
  • Nuku'alofa

  Now, while it is debatable whether a time zone can in more than one group (f.e. Hawaii is in us_zones and in pacific_zones) I can not see any clear mis-classification. The same holds for the other convenience methods provided with the patch.

  I do agree that it would be advantageous to have the time zones grouped not be a mere regular expression but by a more sophisticated approach; nevertheless, the approach in the patch seems to work quite good.

• 

  Pratik May 15th, 2008 @ 11:35 AM

  • Assigned user set to “Geoff Buesing”
• 

  Geoff Buesing May 15th, 2008 @ 03:14 PM

  1. If we're to add more convenience methods for grouping time zones, we need to justify each one we add: it should address a common, real-world application need.

  So, even if we can agree on which time zones to group together as Pacific zones, is there actually a common need for a Pacific time zone grouping?

  2. Any convenience method we add would need to be thread-safe -- see how we're preloading TimeZone.all for an example

• You flagged this item as spam.
  

  Martin Eisenhardt May 15th, 2008 @ 04:25 PM

  Hi Geoff,

  ad 1.) I agree that a Pacific time zone group is probably nothing that will be used very much. I do see much uses for european_zones or asian_zones.

  At the same time, I advocate strongly against leaving one world region out just because the corresponding method will not be used very much.

  Please see again the comments on RailsCasts 106 (Time zones in Rails 2.1) to see what kind of impression that leaves: People feel left out and not cared about, especially in the current state where there is only a us_zones method. Clearly, Rails is used outside the U.S., and a lonely us_zones method conveys a certain US-centric point of view. I am not sure whether this will actually help things.

  I am sure that you will decide what is best for Rails as a whole and include (or not include!) just the methods that make the most sense.

  ad 2.) I already did a thread-safe version and will upload a new patch with this comment. Thanks for the advice.

  Kind regards, Martin

  • 0001-Add-convenience-methods-for-time-zones-outside-the-U.patch 4.4 KB

• You flagged this item as spam.
  

  Bryce May 15th, 2008 @ 10:26 PM

  Maybe we could stick with continents, i.e europe_zones, asia_zones, australian_zones, african_zones, south_american_zones, north_american_zones (because there's also Canada)

• 

  Geoff Buesing May 16th, 2008 @ 04:36 AM

  To be clear, I'm not advocating leaving out one world region. I'm advocating that we only add additional TimeZone convenience methods to core if we can prove there's a common need for them in real-world applications. Without that justification, we're just putting zones into buckets for no real reason other than to appease one upset person in a comments thread.

  Keep in mind that, the us_zones method is a convenience method used only to make it easy to float US zones to the top of the time zone select. But you're not limited to just these zones -- the time_zone_select api allows you to pass in any set of zones you need. A globalization plugin could easily hook into this and provide an extended set of convenience methods.

• You flagged this item as spam.
  

  Bryce May 16th, 2008 @ 06:16 AM

  I'm advocating that we only add additional TimeZone convenience methods to core if we can prove there's a common need for them in real-world applications.

  Definitely, however I think the key disagreement is the fact that /only/ U.S zones are included by default - If anything, there should be at least a helper function for european zones - being a very populated area.

  the time_zone_select api allows you to pass in any set of zones you need

  I'll have to read more about this specific API

• 

  Geoff Buesing May 16th, 2008 @ 02:37 PM

  Here's an example of how to float a custom grouping of zones to the top of time_zone_select:

  # config/initializers/time_zones.rb:
  MY_ZONES = [ TimeZone['Alaska'], TimeZone['Hawaii'] ]
  
  # app/views/users/_form.html.erb:
  <%= f.time_zone_select :time_zone, MY_ZONES %>
  

• You flagged this item as spam.
  

  Martin Eisenhardt May 16th, 2008 @ 03:19 PM

  OK, I got the point: This functionality is better implemented in an i18n plugin. No worries.

  But: You should really think about what kind of impression it makes if you

  • have a us_zones method and say this is justified because of many use cases, and - at the same time -
  • do not include similar methods for other countries or at least world regions, without leaving anyone out in the rain.

  The impression on non-US devs is - to say the least - a bit irritating. Of course, it is not difficult to implement the functionality outside of Rails in your own app, but you got the nagging feeling that Rails is somewhat US-centric and not overly interested in making things easy for devs in other geographic locations.

  Plus, the comment I linked to above contains not only one upset dev, but several; plus, it is only one blogpost with such comments.

  I can certainly understand that you do not want to bloat Rails with loads of convenience methods, especially given the critique that Rails is very large already.

  Well, I will simply implement this functionality in my apps, no problem there.

• 

  Geoff Buesing May 17th, 2008 @ 08:17 PM

  The us_zones method is potentially helpful to devs all over the world, not just US-based devs.

  Proof in point: the us_zones method was first added to the framework by a non-US dev:

  http://dev.rubyonrails.org/chang...

  :)

• You flagged this item as spam.
  

  Martin Eisenhardt May 18th, 2008 @ 11:29 AM

  OK, Geoff, point taken ... :-D

• You flagged this item as spam.
  

  Adam S May 19th, 2008 @ 08:16 AM

  Can't help chipping in here:

  It's irrelevant whether the us_zones choice came from a non-US resident. Initially it seemed like a nice addition, but rejection of this patch reeks slightly of US-centricism... I personally would never use the us_zones method, but would use the australian_zones method. Does this count as a point towards real-world need? Should we take a poll? It's probably not exactly the same, but politicians say that for every letter they receive it represents 1000 votes... i.e. the comment(s) on rails casts represents more than one person's view. If they bothered to write it then they are probably not the only one thinking it. There have also been at least 3 other people (not including myself) comment on this ticket saying that they would like to see it implemented.

  I would advocate for "zones_by_regex" and no nation specific code. I honestly can't think of a reason not to implement, either; all by continent, or nothing but great APIs... Really. But of course I don't live in America so maybe i'm just bitter. ;)

• 

  josh May 19th, 2008 @ 12:43 PM

  TimeZone.zones_by_regex(/../) # 30 char
  TimeZone::ZONES ~= /../ # 24 char
  

  Sorry, but I think zones_by_regex does not help really help.

• 

  josh May 19th, 2008 @ 05:47 PM

  Nevermind. Didnt see it was matching the name attr. Geoff, I'd consider adding TimeZone~= And just keep the US zones.

• You flagged this item as spam.
  

  Geoff Buesing May 19th, 2008 @ 07:48 PM

  I like Josh's suggestion -- a TimeZone ~= method could match based on either the #name or the TZInfo identifier in the MAPPING hash.

  The time_zone_select priority zones option could then be enhanced to accept a regex, so this would be possible:

  <%= f.time_zone_select :time_zone, /Australia/ %>

  This seems like a reasonable compromise -- we can make localization easier, without adding a bunch of methods to the API.

  If someone's interested in pulling together a patch for this idea, I'd be happy to commit it.

• 

  Pratik May 20th, 2008 @ 12:58 PM

  Removing "patch" tag.

• You flagged this item as spam.
  

  oddlyzen May 30th, 2008 @ 06:34 PM

  So, is someone working on this -- or not? Discussion seemed to die right after a reasonable solution was suggested... :)

• You flagged this item as spam.
  

  Ernie Miller June 24th, 2008 @ 11:46 PM

  I just took a stab at it. Have a look at #479 and let me know what you think?

• You flagged this item as spam.
  

  Ernie Miller June 25th, 2008 @ 04:29 AM

  Reposting patch here. Thanks to Jeremy Kemper for teaching me to read. ;)

• You flagged this item as spam.
  

  Ernie Miller June 25th, 2008 @ 04:29 AM

  Reposting patch here. Thanks to Jeremy Kemper for teaching me to read. ;)

• You flagged this item as spam.
  

  Ernie Miller June 25th, 2008 @ 04:36 AM

  Ugh. No way to remove a previous comment? Sorry. :/

• You flagged this item as spam.
  

  Ernie Miller June 25th, 2008 @ 04:50 AM

  Hm. And now, it deleted both duplicate attachments when it wasn't responding to clicks before. Reposting the file. Sorry about that mess.

• 

  Geoff Buesing June 25th, 2008 @ 03:14 PM

  • Tag set to “activesupport, convenient, enhancemen, helper, i18n, patch, timezonece”

  Ernie,

  I think the =~ method should be on the TimeZone instance, not the class, so that we're mirroring the semantics of String#=~

  We could then do the on-the-fly lookup in time_zone_select like so:

  # TODO: make this work with options[:model]
  if priority_zones.is_a?(Regexp)
    priority_zones = TimeZone.all.find_all {|z| z =~ priority_zones}
  end
  

  In the time_zone_select tests, instead of replicating the MAPPING hash etc in MockTimeZone, you could try stubbing out TimeZone.all so that it returns an array of TimeZone objects, and stub some of those objects to return true for =~.

• 

  Geoff Buesing June 25th, 2008 @ 03:21 PM

  • Tag changed from “activesupport, convenient, enhancemen, helper, i18n, patch, timezonece” to “actionpack, activesupport, i18n, patch, timezone”

  Hmmm why did Lighthouse say I just added all of those tags?

• You flagged this item as spam.
  

  Ernie Miller June 25th, 2008 @ 06:16 PM

  Geoff,

  Thanks -- Good calls. I'll work on those changes and resubmit, hopefully later today.
  
  

• You flagged this item as spam.
  

  Ernie Miller June 27th, 2008 @ 05:21 PM

  Check out the new version, attached. Is this a bit better? I moved the priority_zones part into time_zone_options_for_select, instead, which allowed me to make it work with options[:model], and used the objects' #names instead of scanning the hash. It seems a little cleaner.

  I went with a true/false return on the =~... Should we perhaps return the zone itself or nil, instead? I could have gone either way with it.

• 

  Jeremy Kemper June 28th, 2008 @ 02:10 AM

  • State changed from “new” to “open”
  • Milestone set to “2.1.1”
• 

  Geoff Buesing June 28th, 2008 @ 06:48 PM

  re: TimeZone#=~

  1. I think returning just true or nil is fine for now

  2. instead of matching tzinfo.name, which will preload the tzinfo instance for every TimeZone, just match against MAPPING[name]

  3. need tests for this method

  re: time_zone_select

  1. makes sense to put the regex matching in time_zone_options_for_select

  2. in the test, you might consider using Mocha to stub out #=~ on MockTimeZone instances, as opposed to adding more logic to MockTimeZone

• 

  Ernie Miller June 29th, 2008 @ 03:34 AM

  • no changes were found...

  • tzselect_regexp.diff 5.5 KB

• You flagged this item as spam.
  

  Ernie Miller June 29th, 2008 @ 03:34 AM

  Done. I sort of feel like I cheated with my MockTimeZone stub, but then again, that's what the added test for TimeZone#=~ is for, I suppose. :)

• 

  Repository June 29th, 2008 @ 07:39 PM

  • State changed from “open” to “resolved”

  (from [d0092dc44d580f4179308c7394d9023098406f79]) Added support for regexp matching of priority zones in time_zone_select [#195 state:resolved]

  http://github.com/rails/rails/co...

• 

  Ryan Bigg October 11th, 2010 @ 12:11 PM

  • Tag cleared.
  • Importance changed from “” to “Low”

  Automatic cleanup of spam.