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

Fixed #4278: String#underscore with digit #4280

Merged

Conversation

makenowjust
Copy link
Contributor

Fixed #4278.

It treats digit characters as downcase characters. But in upcase characters it treats like upcase characters. It works fine.

"CSS3".underscore     # => "css3"
"HTTP1.1".underscore  # => "http1.1"
"3.14IsPi".underscore # => "3.14_is_pi"

Thank you.

Treat digit as downcase character. But in upcase characters it treats
like upcase character. It works fine.
@makenowjust
Copy link
Contributor Author

@bew Updated comments in 328d4ce. Thank you.

@makenowjust
Copy link
Contributor Author

https://travis-ci.org/crystal-lang/crystal/jobs/221621776

Hmm... openssl spec depends on this strange behavior. I think we put _ before V if we need to separate, like NO_SSLV3 to NO_SSL_V3. For example, do you want to convert WMV9 to wm_v9? I say of course 'no'.

NOTE: Original OpenSSL constant name is NO_SSLv3, not NO_SSLV3. v is just lower case letter. It is important thing that we don't respect original constant name already.

@asterite
Copy link
Member

Let's rename openssl constants to match this new behaviour.

Since we are fixing this, what happens if you pass other characters like some of !@#$%^&*() to underscore? I'd include some tests for those edge cases too and make sure they behave like in Ruby's ActiveSupport.

@makenowjust makenowjust force-pushed the fix/string/underscore-with-digit branch from a8ad45b to aba7363 Compare May 16, 2017 07:32
@makenowjust
Copy link
Contributor Author

makenowjust commented May 16, 2017

@asterite Apologize for delay. I renamed OpenSSL constants for changes and updated this branch.

I know ActiveSupport's String#underscore converts "::" to "/" for Rails convention. However I think it is for Rails reason, so I don't implement it for now.

@akzhan
Copy link
Contributor

akzhan commented May 23, 2017

I prefer to use ActiveSupport compatible behavior when it's possible and have no drawbacks.

@makenowjust
Copy link
Contributor Author

I'm sure String#underscore is bad naming if it converts "::" to "/".

@makenowjust
Copy link
Contributor Author

@asterite ping

@mverzilli mverzilli merged commit da48f82 into crystal-lang:master Jun 2, 2017
@mverzilli
Copy link

Thank you @makenowjust !!!

@mverzilli mverzilli added this to the Next milestone Jun 2, 2017
@makenowjust makenowjust deleted the fix/string/underscore-with-digit branch June 2, 2017 13:06
@makenowjust
Copy link
Contributor Author

@mverzilli Can you close #4281?

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

Successfully merging this pull request may close these issues.

6 participants