Revert grep to select since they are not the same #9680

Merged
merged 2 commits into from Mar 31, 2013

Projects

None yet

3 participants

@bdmac
Contributor
bdmac commented Mar 12, 2013

A previous commit swapped out a call to select for a call to grep in
time_zone_options_for_select. This behavior actually causes the
regexp priority option to stop working.

ActiveSupport::TimeZone overrides the =~ operator which is what the
select block was using previously. 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 back to using select with =~.

@bdmac
Contributor
bdmac commented Mar 12, 2013

Here is the commit that this reverts along with some internal monologue from when I was looking into the problem: b4051ed

@Empact
Contributor
Empact commented Mar 12, 2013

Regression test would be good too. (presuming there wasn't a test failure accompanying b4051ed)

@bdmac
Contributor
bdmac commented Mar 12, 2013

There wasn't a test failure but that's because the test was modified and === was stubbed out on the regexp to return something it shouldn't. Will try to come up with a suitable regression test.

@bdmac
Contributor
bdmac commented Mar 12, 2013

I amended my commit to take a stab at a regression test. The test cases are so mocked up though that it's somewhat contrived. It should at least make it clear to the next person that tries to replace the #select call with a #grep call that it won't work in the real world.

@carlosantoniodasilva

@bdmac please issue a revert commit of the original one, then add yours on top of it with a regression test, it's better if they are different commits.

I still wonder whether we should fix AS::TZ instead =(

@bdmac
Contributor
bdmac commented Mar 12, 2013

The problem is that the current implementation of =~ checks two values against the regex. It would appear the fix to get pattern === TZ (for grep to work) would require the addition of a #to_str method on TZ but not sure how we can handle the two values from =~ to maintain parity between the two versions. If we just add to_str that returns @name then we would still be breaking anyone using priority_zones regexes that are matching on the other =~ value. Maybe that's ok?

Sent from Mailbox for iPhone

On Tue, Mar 12, 2013 at 4:56 AM, Carlos Antonio da Silva
notifications@github.com wrote:

@bdmac please issue a revert commit of the original one, then add yours on top of it with a regression test, it's better if they are different commits.

I still wonder whether we should fix AS::TZ instead =(

Reply to this email directly or view it on GitHub:
#9680 (comment)

@bdmac
Contributor
bdmac commented Mar 12, 2013

I added a #to_str on AS::TZ and ran the tests and they still passed including the modified version from b4051ed. I'm still not entirely comfortable whether that's a good indication that we're not "really" breaking any backwards compatibility since there were no tests to catch this problem in the first place. The amount of stubbing happening makes it hard to really see in testing.

@bdmac
Contributor
bdmac commented Mar 18, 2013

Any further word on this PR?

@carlosantoniodasilva

@bdmac It's on my hands now to take a look, thanks.

@bdmac
Contributor
bdmac commented Mar 26, 2013

@carlosantoniodasilva anything I can do to help here? My app makes use of this feature so I'd love to at least see this fix merged as a stop-gap so I can bundle update rails to get my TZ selects working again. :)

@carlosantoniodasilva

@bdmac I'm having some rush days traveling/working here, sorry. Can you separate the changes in two different commits? One is reverting the original so that it's more trackable, other adding your test on top of the revert commit.

Also, not sure if we can use something like real TZ instead of stubbing, or at least make the stub raise an error instead of returning [] because it should never receive ===, wdyt?

@carlosantoniodasilva

Btw, thanks for keeping this up! 😄

bdmac added some commits Mar 27, 2013
@bdmac bdmac Revert grep to select since they are not the same
A previous commit swapped out a call to select for a call to grep in
time_zone_options_for_select. This behavior actually causes the
regexp priority option to stop working.

ActiveSupport::TimeZone overrides the =~ operator which is what the
select block was using previously. 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 back to using select with =~.
1cc991b
@bdmac bdmac Add regression test for TZ grep
Added a regression test that will fail if anyone tries to change
time_zone_select to use grep again thinking it will work when it does
not.
53d68bd
@bdmac
Contributor
bdmac commented Mar 27, 2013

@carlosantoniodasilva split the commit in two and swapped in an exception instead of returning [] as requested.

@carlosantoniodasilva carlosantoniodasilva merged commit 0a16cf1 into rails:master Mar 31, 2013
@carlosantoniodasilva

@bcmac Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment