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

Implement `String#casecmp?` and `Symbol#casecmp?` #4470

Merged
merged 2 commits into from Feb 13, 2017

Conversation

Projects
None yet
3 participants
@kcdragon
Copy link
Contributor

kcdragon commented Feb 2, 2017

  • Adds implementation of casecmp? based on downcasing the string and comparing
  • Adds unit tests for casecmp? from MRI

There is a test failing in each of the tests (the last assertion in each method) I brought over. This is because the MRI implementation (ruby/ruby@ad619e0) of casecmp? relies on passing the :fold option to downcase however that option has not been added yet in JRuby. It is referenced in #4293 though.

I decided to keep the failing tests in for completeness since they will need to be fixed anyway for 2.4 support. I figured that this PR would be fine without those specific cases supported but maybe it would be better to wait until :fold is implemented.

See #4293

Implement `String#casecmp?` and `Symbol#casecmp?`
* Adds implementation of `casecmp?` based on downcasing the string and comparing
* Adds unit tests for `casecmp?` from MRI

See #4293
@kares

This comment has been minimized.

Copy link
Member

kares commented Feb 2, 2017

excellent, any reason why the encoding compatible check is not being implemented (like in MRI) ?

@kares kares added the ruby 2.4 label Feb 2, 2017

@kcdragon

This comment has been minimized.

Copy link
Contributor Author

kcdragon commented Feb 2, 2017

excellent, any reason why the encoding compatible check is not being implemented (like in MRI) ?

@kares

There's no reason. I can add that. Are you referring to something like this being done?

Encoding enc = StringSupport.areCompatible(this, otherStr);
if (enc == null) return runtime.getNil();

https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/RubyString.java#L1572-L1573

@kares

This comment has been minimized.

Copy link
Member

kares commented Feb 3, 2017

yy

@kcdragon

This comment has been minimized.

Copy link
Contributor Author

kcdragon commented Feb 11, 2017

@kares I added the encoding compatibility check.

@headius

This comment has been minimized.

Copy link
Member

headius commented Feb 13, 2017

Looks ok to me!

@headius headius merged commit edbd1d5 into jruby:ruby-2.4 Feb 13, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@kares kares added this to the JRuby 9.2.0.0 milestone Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.