Skip to content

Fixed #4278: String#underscore with digit#4280

Merged
mverzilli merged 4 commits into
crystal-lang:masterfrom
makenowjust:fix/string/underscore-with-digit
Jun 2, 2017
Merged

Fixed #4278: String#underscore with digit#4280
mverzilli merged 4 commits into
crystal-lang:masterfrom
makenowjust:fix/string/underscore-with-digit

Conversation

@makenowjust

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@bew Updated comments in 328d4ce. Thank you.

@makenowjust

Copy link
Copy Markdown
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
Copy Markdown
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

makenowjust commented May 16, 2017

Copy link
Copy Markdown
Contributor Author

@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

akzhan commented May 23, 2017

Copy link
Copy Markdown
Contributor

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

@makenowjust

Copy link
Copy Markdown
Contributor Author

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

@makenowjust

Copy link
Copy Markdown
Contributor Author

@asterite ping

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

Copy link
Copy Markdown

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
Copy Markdown
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