Skip to content

Fix underscore inflector handling of namespaced and adjacent acronyms #14146

Merged
merged 1 commit into from Oct 3, 2014

6 participants

@chewi
chewi commented Feb 21, 2014

The underscore inflector regular expression was not working correctly
for namespaced acronyms that were not entirely uppercased
(e.g. HTTP::RESTful) or adjacent acronyms (e.g. APIRESTful).

I suspect that positive lookbehind would have been used in the
original implementation had it been available in supported Ruby
versions at the time. Now that Rails requires Ruby 1.9.2 or above,
this is no longer an issue.

Thanks to Pawel Kozlowski (khozlov) for highlighting the namespace
issue in #8571.

@senny
Ruby on Rails member
senny commented Feb 26, 2014

/cc @fxn

@robin850
Ruby on Rails member
robin850 commented Mar 2, 2014

(I take the liberty to link to a related patch : #12510)

@fxn fxn was assigned by matthewd Apr 16, 2014
@agios
agios commented Oct 3, 2014

It would be nice to get this fixed, the first pr is already almost 2 years old...
👍

@chewi
chewi commented Oct 3, 2014

Since I posted this a while ago, I'd like to add that I have monkey patched this into a number of my projects and have not seen a single issue.

@matthewd
Ruby on Rails member
matthewd commented Oct 3, 2014

Does this fix something more than ccbb481?

@chewi chewi Fix underscore inflector handling of adjacent acronyms
I suspect that positive lookbehind would have been used in the
original implementation had it been available in supported Ruby
versions at the time. Now that Rails requires Ruby 1.9.2 or above,
this is no longer an issue.

This fixes #14146 for acronyms such as APIRESTful. This technique also
addresses namespaced acronyms that are not entirely uppercased. This
was broken when the commit was originally written but has since been
fixed in ccbb481. The latter does not deal with adjacent acronyms so
this commit wins.
6a8464f
@chewi
chewi commented Oct 3, 2014

Just tried your fix against mine for the various test cases. Yours deals with the "namespaced" acronyms but not the "adjacent" acronyms. My fix deals with both, including the test case you added. I have now rebased and pushed.

@matthewd matthewd merged commit 6a8464f into rails:master Oct 3, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@matthewd
Ruby on Rails member
matthewd commented Oct 3, 2014

👍. I've merged it with a slightly more minimal change to the spelling, just because paranoia. .. and because it makes it easier to see exactly what had to change, if we do run into any weird compatibility issues.

But this seems quite safe -- for strings of this particular form, the previous implementation was mangling them badly, so it seems unreasonable for people to have been depending on same. And I think it's better to get this change into the same release as ccbb481, rather than make similarly incremental adjustments in two releases in a row.

/cc @fxn

(merged as cf66278, then fixed for 1.9 in cad6359)

@chewi
chewi commented Oct 4, 2014

I didn't realise my version could be made simpler using \b. I thought I'd already tried that actually but it seems to work. Thanks!

@sachin004 sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014
@chewi chewi Fix underscore inflector handling of adjacent acronyms
I suspect that positive lookbehind would have been used in the
original implementation had it been available in supported Ruby
versions at the time. Now that Rails requires Ruby 1.9.2 or above,
this is no longer an issue.

This fixes #14146 for acronyms such as APIRESTful. This technique also
addresses namespaced acronyms that are not entirely uppercased. This
was broken when the commit was originally written but has since been
fixed in ccbb481. The latter does not deal with adjacent acronyms so
this commit wins.
e9f5ac3
@divineforest divineforest added a commit that referenced this pull request Jan 12, 2015
@chewi chewi Fix underscore inflector handling of adjacent acronyms
I suspect that positive lookbehind would have been used in the
original implementation had it been available in supported Ruby
versions at the time. Now that Rails requires Ruby 1.9.2 or above,
this is no longer an issue.

This fixes #14146 for acronyms such as APIRESTful. This technique also
addresses namespaced acronyms that are not entirely uppercased. This
was broken when the commit was originally written but has since been
fixed in ccbb481. The latter does not deal with adjacent acronyms so
this commit wins.
f690022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.