Skip to content
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

Closed

Conversation

olbat
Copy link
Contributor

@olbat olbat commented Apr 13, 2017

The String#underscore method does not behave correctly when handling
strings containing non-letter characters such as digits (i.e. "FOO123")

The String#underscore method does not behave correctly when handling
strings containing non-letter characters such as digits (i.e. "FOO123")
@makenowjust
Copy link
Contributor

I opened #4280 to fix #4278 already and your pull request doesn't work for now. Hmm... 🤔

@olbat
Copy link
Contributor Author

olbat commented Apr 13, 2017

Yes, sorry I didn't see you PR before pushing, I've put a message in the issue's page ...

_Update_: the CI failure may be related to some code that was relying on the previous behavior of `String#underscore` ?

@@ -3314,7 +3314,7 @@ class String
if mem
if char == '_'
# case 3
else
elsif upcase || downcase
Copy link
Contributor

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".

Copy link
Contributor

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 ||.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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"

Copy link
Contributor Author

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)

@makenowjust
Copy link
Contributor

makenowjust commented Apr 13, 2017

@olbat I can say 'yes', and 'no' also. At first, your implementation is totally broken. Please see my comment.

openssl spec depends String#underscore strange behavior. You (and I) invoked it.

@mverzilli
Copy link

Closing in favor of #4278

@mverzilli mverzilli closed this Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants