Browse files

Use #grep to filter priority zones when a regexp is given

  • Loading branch information...
1 parent 953e19e commit b4051edf841c8a6780df9af7afa9892bfd811c79 @carlosantoniodasilva carlosantoniodasilva committed Feb 21, 2013
@@ -565,7 +565,7 @@ def time_zone_options_for_select(selected = nil, priority_zones = nil, model = :
if priority_zones
if priority_zones.is_a?(Regexp)
- priority_zones = { |z| z =~ priority_zones }
+ priority_zones = zones.grep(priority_zones)
zone_options.safe_concat options_for_select(convert_zones[priority_zones], selected)
@@ -1086,11 +1086,13 @@ def test_time_zone_select_with_priority_zones
def test_time_zone_select_with_priority_zones_as_regexp
@firm ="D")
+ priority_zones = /A|D/
@fake_timezones.each_with_index do |tz, i|
- tz.stubs(:=~).returns( || i == 3)
+ priority_zones.stubs(:===).with(tz).returns( || i == 3)
- html = time_zone_select("firm", "time_zone", /A|D/)
+ html = time_zone_select("firm", "time_zone", priority_zones)
assert_dom_equal "<select id=\"firm_time_zone\" name=\"firm[time_zone]\">" +
"<option value=\"A\">A</option>\n" +
"<option value=\"D\" selected=\"selected\">D</option>" +

3 comments on commit b4051ed


bdmac replied Mar 12, 2013

This commit actually breaks things that used to work. Still trying to figure out why exactly but from rails console try:

2.0.0p0 :001 > zones = ActiveSupport::TimeZone.all
2.0.0p0 :002 > zones.grep(/US/)
 => [] 
2.0.0p0 :004 > { |z| z =~ /US/ }
 => [#<ActiveSupport::TimeZone:0x007ff30dc98c38 @name="Pacific Time (US & Canada)", @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/Los_Angeles>, @current_period=#<TZInfo::TimezonePeriod: #<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1362909600>,#<TZInfo::TimezoneOffsetInfo: -28800,3600,PDT>>,#<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1383469200>,#<TZInfo::TimezoneOffsetInfo: -28800,0,PST>>>>, #<ActiveSupport::TimeZone:0x007ff30f514a28 @name="Mountain Time (US & Canada)", @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/Denver>, @current_period=#<TZInfo::TimezonePeriod: #<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1362906000>,#<TZInfo::TimezoneOffsetInfo: -25200,3600,MDT>>,#<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1383465600>,#<TZInfo::TimezoneOffsetInfo: -25200,0,MST>>>>, #<ActiveSupport::TimeZone:0x007ff30f514848 @name="Central Time (US & Canada)", @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/Chicago>, @current_period=#<TZInfo::TimezonePeriod: #<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1362902400>,#<TZInfo::TimezoneOffsetInfo: -21600,3600,CDT>>,#<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1383462000>,#<TZInfo::TimezoneOffsetInfo: -21600,0,CST>>>>, #<ActiveSupport::TimeZone:0x007ff30f514578 @name="Eastern Time (US & Canada)", @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/New_York>, @current_period=#<TZInfo::TimezonePeriod: #<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1362898800>,#<TZInfo::TimezoneOffsetInfo: -18000,3600,EDT>>,#<TZInfo::TimezoneTransitionInfo: #<TZInfo::TimeOrDateTime: 1383458400>,#<TZInfo::TimezoneOffsetInfo: -18000,0,EST>>>>] 

As you can see the old version using select works for a priority_zones argument of /US/ but the grep version returns nothing. Will continue looking at this tomorrow.


bdmac replied Mar 12, 2013

Well it's because ActiveSupport::TimeZone overrides the =~ operator. Enumerable#grep checks pattern === element and in this case that would be /US/ === ActiveSupport::TimeZone which does not work because ActiveSupport::TimeZone does not supply an implicit converting to_str method, only an explicit to_s method. It would be impossible to provide a to_str method that behaves identically to the =~ method provided on ActiveSupport::TimeZone so the only option is to revert this change I believe.

@bdmac interesting. Can you provide a test case that shows the issue? Just open a pull request and cc me, and I'll take a look later. If necessary, we can revert, no problem, but at least we'll have a regression test for that. Thanks.

Please sign in to comment.