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

Simply turn into a string and downcase #125

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Conversation

nobu
Copy link
Member

@nobu nobu commented Sep 26, 2019

Symbol#to_s returns frozen string now.

`Symbol#to_s` returns frozen string now.
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you for creating the PR.

@yuki24
Copy link
Member

yuki24 commented Sep 26, 2019

Thanks!

@yuki24 yuki24 merged commit 54c1baa into ruby:master Sep 26, 2019
@MSP-Greg
Copy link

To all,

Thanks for the fix. JFYI, this is causing errors in both spec & test-all when run in Ruby master without --disable=did_you_mean. Probably the reason for the PR.

Due to:
ruby/ruby@6ffc045
[EXPERIMENTAL] Make Symbol#to_s return a frozen String
Thu Sep 26 10:23:02 2019 +0200

@eregon
Copy link
Member

eregon commented Sep 27, 2019

@MSP-Greg Could you point to a failure in a CI log? Or is it only happening locally?

@MSP-Greg
Copy link

@eregon

ruby/ruby builds are from the build folder, so they may not have did_you_mean enabled, as it is a bundled gem. ruby-loco runs tests from install, so it is enabled. See below:

NoMethodError#message calls receiver.inspect only when calling Exception#message FAILED
Expected [:inspect_called, :inspect_called]
 to equal [:inspect_called]

/ruby/spec/ruby/core/exception/no_method_error_spec.rb:86:in `rescue in block (2 levels) in <top (required)>'
/ruby/spec/ruby/core/exception/no_method_error_spec.rb:80:in `block (2 levels) in <top (required)>'
/ruby/spec/ruby/core/exception/no_method_error_spec.rb:45:in `<top (required)>'
  1) Error:
TestRDocRIDriver#test_display_name_not_found_class:
FrozenError: can't modify frozen String: ""
    /install/lib/ruby/gems/2.7.0/gems/did_you_mean-1.3.0/lib/did_you_mean/spell_checker.rb:47:in `downcase!'
    /install/lib/ruby/gems/2.7.0/gems/did_you_mean-1.3.0/lib/did_you_mean/spell_checker.rb:47:in `normalize'
    /install/lib/ruby/gems/2.7.0/gems/did_you_mean-1.3.0/lib/did_you_mean/spell_checker.rb:13:in `correct'
    /install/lib/ruby/2.7.0/rdoc/ri/driver.rb:1286:in `lookup_method'

I patched the method:

    def normalize(str_or_symbol) #:nodoc:
      str_or_symbol.to_s.downcase.tr("@", "").dup
    end

As you know, it only affects people using very new versions of ruby-head. Didn't you do the commit?

@yuki24
Copy link
Member

yuki24 commented Sep 28, 2019

@MSP-Greg Interesting. I wonder if there is a case where the #downcase method also returns a frozen string. If that's the case, I'd be happy to apply your patch above.

@MSP-Greg
Copy link

MSP-Greg commented Sep 28, 2019

@yuki24

I've been writing/refactoring too many tests lately. Hence, write bullet-proof code.

where the #downcase method also returns a frozen string

Well, a few days ago, Symbol#to_s wasn't frozen...

It's probably fine the way it is, and @nobu is certainly aware of what may be changing in the future...

EDIT: #downcase really isn't comparable to #to_s, as #to_s normally initializes a new string, where #downcase modifies an existing one.

@nobu
Copy link
Member Author

nobu commented Sep 28, 2019

@MSP-Greg You might want to write #downcase!, I think.

@nobu nobu deleted the frozen-Symbol#to_s branch September 28, 2019 06:14
@eregon
Copy link
Member

eregon commented Sep 28, 2019

Actually, the did_you_mean tests failed on https://github.com/ruby/ruby/runs/236956239 but nobody noticed since that's run by ignored the exit status (https://github.com/ruby/ruby/blob/550a6a6bc1be4ff9aa6b65f6ad9b45c3fa2d1344/.github/workflows/ubuntu.yml#L83)

@eregon
Copy link
Member

eregon commented Sep 28, 2019

@yuki24 Could you make a release of did_you_mean with this fix? 🙏
This seems to be the only way to make the did_you_mean tests in ruby/ruby's CI pass.

@MSP-Greg
Copy link

MSP-Greg commented Sep 28, 2019

@nobu

You might want to write #downcase!, I think.

I'm probably mistaken, but isn't that true iif str is the final return value?

I ran the tests locally without the final .dup call shown above, and they ran fine. IOW, the method was simply str_or_symbol.to_s.downcase.tr '@', ''

@yuki24
Copy link
Member

yuki24 commented Sep 29, 2019

released as part of v1.3.1.

@eregon
Copy link
Member

eregon commented Sep 29, 2019

@yuki24 Thank you, did_you_mean tests pass on ruby/ruby now: ruby/ruby#2503

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

Successfully merging this pull request may close these issues.

4 participants