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

ShadowedException cop cause warning: constant ::TimeoutError is deprecated #6675

Closed
ShockwaveNN opened this issue Jan 16, 2019 · 6 comments
Closed
Labels
feature request good first issue Easy task, suitable for newcomers to the project

Comments

@ShockwaveNN
Copy link
Contributor

I'm not 100% sure this is an issue, but I don't think this is correct behaviour

Steps to reproduce the problem

  1. Create test.rb file with
def foo
  puts 'bar'
rescue TimeoutError
  puts 'foo'
end

  1. run rubocop test.rb

Expected behavior

Some cop should warn me about deprecation of TimeoutError

Actual behavior

Inspecting 1 file
/home/lobashov/.rvm/gems/ruby-2.5.3/gems/rubocop-0.62.0/lib/rubocop/cop/lint/shadowed_exception.rb:120: warning: constant ::TimeoutError is deprecated
.

1 file inspected, no offenses detected

RuboCop version

$ [bundle exec] rubocop -V
0.62.0 (using Parser 2.5.3.0, running on ruby 2.5.3 x86_64-linux)
@pocke
Copy link
Collaborator

pocke commented Jan 16, 2019

Thank you for reporting issue!

https://github.com/rubocop-hq/rubocop/blob/7abc4badefca71f5fd9cb3f9b0e8e0f78801a51e/lib/rubocop/cop/lint/shadowed_exception.rb#L115
This cop evaluates constant name, and TimeoutError warns deprecation of itself when it's evaluated.

require 'timeout'
p TimeoutError # => warning: constant ::TimeoutError is deprecated

I'm not sure we avoid this warning, If we supress the warning, we need a comment why it is supressed.

@pocke
Copy link
Collaborator

pocke commented Jan 16, 2019

By the way, we can add a cop like Lint/DeprecatedTimeoutError for more descriptive warning message.
It should says "TimeoutError is deprecated. Use Timeout::Error instead.".

I think it is the best way if MRI's warning includes "Use Timeout::Error instead.".
But it looks difficult. It uses deprecate_constant method to add the deprecation, but it does not have any interface to set "instead".
https://github.com/ruby/ruby/blob/d7976d145193379d8c43f33e9a5714292a40d994/lib/timeout.rb#L125-L130

@ShockwaveNN
Copy link
Contributor Author

@pocke Yeah, I fought that adding cop about this exact message is best solution for this situation

@rrosenblum
Copy link
Contributor

It really is a shame that Ruby's message only gives the deprecation and does not supply the replacement.

I'm not sure we avoid this warning, If we supress the warning, we need a comment why it is supressed.

silence_warnings do
  # Avoid printing deprecation warnings about constants
  converted << Kernel.const_get(exception)
end

This would prevent the warning from showing up when RuboCop runs. I think it would make sense for us to silence the warning since the deprecation warning doesn't have anything to do with the RuboCop code itself.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 29, 2019

It really is a shame that Ruby's message only gives the deprecation and does not supply the replacement.

Indeed. Found another case of this recently. 🙁

This would prevent the warning from showing up when RuboCop runs. I think it would make sense for us to silence the warning since the deprecation warning doesn't have anything to do with the RuboCop code itself.

I agree.

@Drenmi Drenmi added the good first issue Easy task, suitable for newcomers to the project label Feb 5, 2019
@elmasantos
Copy link
Contributor

Hey, I just opened a PR with the changes required in order to avoid showing deprecation warnings about constants.

I think it would make sense for us to silence the warning since the deprecation warning doesn't have anything to do with the RuboCop code itself.

I agree with this, and I'm not sure if the feature described by @pocke is really needed, but if it is, maybe should be done in a differente branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request good first issue Easy task, suitable for newcomers to the project
Projects
None yet
Development

No branches or pull requests

5 participants