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

Stop a duplicate definition warning #123

Merged
merged 1 commit into from
Nov 14, 2016

Conversation

michaelherold
Copy link
Contributor

When running ruby with RUBYOPT="-w" or with RSpec running with
warnings turned on, these errors were being thrown:

slack-ruby-client/lib/slack/real_time/concurrency/celluloid.rb:45: warning: method redefined; discarding old close
ruby-2.3.1/lib/ruby/2.3.0/forwardable.rb:187: warning: previous definitionof close was here
slack-ruby-client/lib/slack/real_time/concurrency/celluloid.rb:61: warning: method redefined; discarding old write
ruby-2.3.1/lib/ruby/2.3.0/forwardable.rb:187: warning: previous definitionof write was here

This change removes the delegators that were present, since the wrappers
call them directly anyway.

When running ruby with `RUBYOPT="-w"` or with RSpec running with
warnings turned on, these errors were being thrown:

```
slack-ruby-client/lib/slack/real_time/concurrency/celluloid.rb:45: warning: method redefined; discarding old close
ruby-2.3.1/lib/ruby/2.3.0/forwardable.rb:187: warning: previous definitionof close was here
slack-ruby-client/lib/slack/real_time/concurrency/celluloid.rb:61: warning: method redefined; discarding old write
ruby-2.3.1/lib/ruby/2.3.0/forwardable.rb:187: warning: previous definitionof write was here
```

This change removes the delegators that were present, since the wrappers
call them directly anyway.
@dangerpr-bot
Copy link

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

@dblock dblock merged commit 5f69a2f into slack-ruby:master Nov 14, 2016
@dblock
Copy link
Collaborator

dblock commented Nov 14, 2016

Thanks @michaelherold. Wonder whether we should run rspec with -w in tests and fail on warnings?

@michaelherold michaelherold deleted the silence-warnings branch November 14, 2016 14:50
@michaelherold
Copy link
Contributor Author

I typically try to run with this configuration:

RSpec.configure do |config|
  config.warnings = true
end

That makes a lot of things pop in the library (mostly uninitialized instance variables), which is helpful from a code quality standpoint.

However, in this case, there are a lot of warnings from the picky gem as well, so it will definitely muddle the RSpec output, at least until we go fix those issues in the related library.

Do you think that would be worth it? It at least shows where the squeaky wheels are.

@dblock
Copy link
Collaborator

dblock commented Nov 14, 2016

Probably not, thanks @michaelherold.

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

Successfully merging this pull request may close these issues.

3 participants