-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
Builds fail fast on any ruby warnings #741
base: main
Are you sure you want to change the base?
Conversation
ruby 2.x emitted warning: instance variable @logger not initialized
JRuby emits the warning: ObjectMethods#method accesses caller method's state and should not be aliased. see ruby/ostruct#40 for a discussion about why JRuby emits this warning. We suppress the warning by extending Warning to ignore the message.
Thank you. I'm broadly in favour of this. I've added some comments below and in some review comments. I've fixed the formatting issues you fixed in While I understand why the commits are in the order they are, I think I'd prefer the commits with the warning fixes to come before the changes to the CI build which catches them, i.e. so all the tests pass in each commit.
This seems like quite a fragile way to detect warnings. Is there definitely no better way to do this. I wonder if it's worth looking around at what other projects do? I haven't thought this through, but could we globally patch
Does the absence of this gem actually trigger a warning at the moment?
Thanks for being so thorough with the testing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See review comments and comment on PR description.
@@ -36,6 +34,10 @@ jobs: | |||
- run: gem --version | |||
- run: bundle --version | |||
- run: bundle install --gemfile=<< parameters.gemfile >> | |||
- run: | | |||
if [ "<< parameters.docker-image >>" != "ruby:2.2" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works I think it might be slightly more expressive to use > "ruby:2.2"
instead of != "ruby:2.2"
.
@@ -7,6 +7,7 @@ group :development do | |||
gem 'introspection', '~> 0.0.1' | |||
gem 'minitest' | |||
gem 'rake' | |||
gem 'rdoc' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, this change is orthogonal to the other changes in this commit. If so, can you split it out into a separate explanatory commit?
module JRubyAliasMethodWarningFilter | ||
def warn(message, *args) | ||
super unless /ObjectMethods#method accesses caller method's state and should not be aliased/.match?(message) | ||
end | ||
end | ||
|
||
# using public_send as calling extend directly causes YARD to warn about Undocumentable mixin | ||
Warning.public_send(:extend, JRubyAliasMethodWarningFilter) # rubocop:disable Lint/SendWithMixinArgument, Style/SendWithLiteralMethodName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit ugly. Could we avoid the public_send
workaround (and associated comment) and the disabling of two cops by using an approach more like this?
As an aside, I wonder if it might separately be worth trying to extract this pattern somewhere. However, if so, I'd prefer to do that in a separate PR.
Closes #729
Introduces and uses a reusable command in CircleCI config for failing fast on warnings. Addresses resulting warnings that originally caused builds to fail with the updated configuration.
Changes:
Benefits:
Testing:
The CI pipeline has been tested to ensure that it correctly fails on warnings for the specified Ruby versions.