Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for inflector's incorrect camelCase replacement for acronyms #8156

Merged
merged 2 commits into from

5 participants

@fredwu

Description of this bug: #8015

@fredwu

PR for 3.2 branch: #8155

@steveklabnik
Collaborator

/cc @fxn

@fredwu

Hi @fxn, would you mind taking a look at this PR as it's been quite a while and it'd be awesome if this is going to get addressed. :)

@fxn
Owner

Can you please rebase this one? We'll apply.

@fredwu

Thanks @fxn , rebased!

@strzalek
Collaborator

Awesome! Thanks @fredwu and @fxn

:metal:

@fxn fxn merged commit 867dc17 into rails:master
@macksmind

This caused breakage in actionpack/test/controller/force_ssl_test.rb, and will likely break production apps as well. Controllers such as ForceSSLOnlyAction now have a path of /force_sslonly_action when previously it would have been /force_ssl_only_action.

@steveklabnik
Collaborator

@macksmind you are awesome. I started getting errors on the rails-api build for this exact situation, and hadn't found the time to track it down.

@fxn
Owner

Have to be reverted then, the example reported in the original PR needs a fix, but the patch is not good.

I am out though, Steve could you revert please?

@steveklabnik
Collaborator

Yes.

@steveklabnik steveklabnik referenced this pull request from a commit
@steveklabnik steveklabnik Revert "Merge pull request #8156 from fredwu/acronym_fix-master"
This reverts commit 867dc17, reversing
changes made to 9a421aa.

This breaks anyone who's using ForceSSL: https://travis-ci.org/rails-api/rails-api/jobs/5556065

Please see comments on #8156 for some discussion.
feaa6e2
@steveklabnik
Collaborator

Reveted in feaa6e2

@fxn
Owner

Thanks!

@fredwu

I'm taking a look at this and will fix my patch.

@fredwu fredwu referenced this pull request from a commit in fredwu/rails
@fredwu fredwu Fixed a bug where the inflector would replace camelCase strings and d…
…isregarding specified acronyms, fixes #8015, closes #8156
7624b86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 16, 2013
  1. @fredwu

    Fixed a bug where the inflector would replace camelCase strings and d…

    fredwu authored
    …isregarding specified acronyms, fixes #8015
  2. @fredwu
This page is out of date. Refresh to see the latest.
View
4 activesupport/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* Fixed a bug in Inflector#underscore where acroynms are incorrectly parsed as camelCases.
+
+ *Fred Wu*
+
* Fix deletion of empty directories in ActiveSupport::Cache::FileStore.
*Charles Jones*
View
2  activesupport/lib/active_support/inflector/methods.rb
@@ -91,7 +91,7 @@ def underscore(camel_cased_word)
word = camel_cased_word.to_s.dup
word.gsub!('::', '/')
word.gsub!(/(?:([A-Za-z\d])|^)(#{inflections.acronym_regex})(?=\b|[^a-z])/) { "#{$1}#{$1 && '_'}#{$2.downcase}" }
- word.gsub!(/([A-Z\d]+)([A-Z][a-z])/,'\1_\2')
+ word.gsub!(/(?!#{inflections.acronym_regex})\b([A-Z\d]+)([A-Z][a-z])/,'\1_\2')
word.gsub!(/([a-z\d])([A-Z])/,'\1_\2')
word.tr!("-", "_")
word.downcase!
View
2  activesupport/test/inflector_test.rb
@@ -167,11 +167,13 @@ def test_acronyms_camelize_lower
def test_underscore_acronym_sequence
ActiveSupport::Inflector.inflections do |inflect|
inflect.acronym("API")
+ inflect.acronym("APIs")
inflect.acronym("JSON")
inflect.acronym("HTML")
end
assert_equal("json_html_api", ActiveSupport::Inflector.underscore("JSONHTMLAPI"))
+ assert_equal("namespaced/apis", ActiveSupport::Inflector.underscore("Namespaced::APIs"))
end
def test_underscore
Something went wrong with that request. Please try again.