-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: String#underscore with non-letter char #4278 #4281
Fix: String#underscore with non-letter char #4278 #4281
Conversation
The String#underscore method does not behave correctly when handling strings containing non-letter characters such as digits (i.e. "FOO123")
Yes, sorry I didn't see you PR before pushing, I've put a message in the issue's page ...
|
@@ -3314,7 +3314,7 @@ class String | |||
if mem | |||
if char == '_' | |||
# case 3 | |||
else | |||
elsif upcase || downcase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You intended last_is_upcase
instead of upcase
correctly, didn't you?
This is not the perfect though. For example, "3.14IsPi"
should be converted to "3.14_is_pi"
. But your implementation converts `"3.14is_pi".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry. It is my mistake. I was misled ||
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure of how "123ABC" should be converted (if "ABC123" has to be converted as "abc123", why make a difference for "123ABC" ?).
The best to do for now is probably to wait for an answer of the core devs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best to do for now is probably to wait for an answer of the core devs ?
Yes.
I imitated rails (activesupport) behavior:
$ ruby -rrails -e 'p "123ABC".underscore'
"123_abc"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I saw that Rails behaves this way too ...
Anyway, I'm not sure it's consistent in Rails either, the doc says As a rule of thumb you can think of underscore as the inverse of camelize
so in that case, the camelized version of "123abc" should be "123Abc" but it is "123abc".
That's a lot of discussions for such a small problem :)
I prefer the way it's done in Rails but it feels strange in some ways too ...
(Sorry again for the double PR, I'll close this one as soon as we have an answer from the devs)
@olbat I can say 'yes'
|
Closing in favor of #4278 |
The String#underscore method does not behave correctly when handling
strings containing non-letter characters such as digits (i.e. "FOO123")