Skip to content

Stop a duplicate definition warning#123

Merged
dblock merged 1 commit intoslack-ruby:masterfrom
michaelherold:silence-warnings
Nov 14, 2016
Merged

Stop a duplicate definition warning#123
dblock merged 1 commit intoslack-ruby:masterfrom
michaelherold:silence-warnings

Conversation

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

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