Add Ruby 3.2 to CI and update RuboCop #107
Conversation
panthomakos
left a comment
panthomakos
left a comment
There was a problem hiding this comment.
Thanks for contributing! Sorry for the delay in responding. I have a few suggestions and questions.
timezone.gemspec
Outdated
| s.add_development_dependency('rake', '~> 12') | ||
| s.add_development_dependency('rubocop', '= 0.51') | ||
| s.add_development_dependency('rubocop', '>= 0.51') | ||
| s.add_development_dependency('rubocop-performance') |
There was a problem hiding this comment.
This should have a version lock as well.
|
So I'm getting a bundler segfault on Ruby 2.2, but everything else is green with no warnings. Not sure if you want to maintain support for 2.2 at this point. |
|
@panthomakos For what it's worth, rerunning the 2.2 job on my fork caused it to pass. |
| assert Timezone.fetch('Australia/Sydney').valid? | ||
|
|
||
| # Explicitly testing block syntax, so disable Cop | ||
| # rubocop:disable Style/RedundantFetchBlock |
| config.username = 'timezone' | ||
| config.request_handler = HTTPTestClientFactory.new(body) | ||
| yield config if block_given? | ||
| yield config if block |
There was a problem hiding this comment.
To keep consistency with the code should we use block_given? here?
There was a problem hiding this comment.
This is a Rubocop fix - https://msp-greg.github.io/rubocop-performance/RuboCop/Cop/Performance/BlockGivenWithExplicitBlock.html
I'm not sure why the previous version would be preferable.
| config.api_key = 'MTIzYWJj' | ||
| config.request_handler = HTTPTestClientFactory.new(body) | ||
| yield config if block_given? | ||
| yield config if block |
There was a problem hiding this comment.
This is a Rubocop fix - https://msp-greg.github.io/rubocop-performance/RuboCop/Cop/Performance/BlockGivenWithExplicitBlock.html
I'm not sure why the previous version would be preferable.
|
|
||
| assert_equal Timezone::Lookup::Test, | ||
| Timezone::Lookup.lookup.class | ||
| Timezone::Lookup.lookup.class |
There was a problem hiding this comment.
Is there a cop changing this behavior and can we revert to the previous behavior?
There was a problem hiding this comment.
This is a spacing lint.
| 'lookups.' | ||
|
|
||
| s.files = `git ls-files`.split("\n") | ||
| s.test_files = `git ls-files -- {test,spec,features}/*`.split("\n") |
There was a problem hiding this comment.
https://docs.rubocop.org/rubocop/cops_gemspec.html#gemspecdeprecatedattributeassignment - because it an unused argument at this point.
| s.rdoc_options = ['--charset=UTF-8'] | ||
| s.require_paths = ['lib'] | ||
|
|
||
| s.required_ruby_version = '>= 2.2' |
There was a problem hiding this comment.
I am not objected to dropping 2.2 support, but it's unclear to me why it stopped working with these changes. If it's feasible to add 3.2 support independently of the RuboCop changes to see if everything still works, I would prefer not to drop 2.2 support unnecessarily.
There was a problem hiding this comment.
Given that Rubocop is being run in the CI pipeline (with a Ruby version of 3.0), I'm not sure if it's feasible. I'm not going to invest the time to determine that, but you should feel free to take whatever you want from this PR and explore that.
|
Also worth noting that it runs green on my fork, including for 2.2 - petergoldstein#1 |
2 participants
This PR adds Ruby 3.2 to the CI matrix and updates RuboCop to a version that can be run with that Ruby version.
Getting everything green required:
Timezone::Lookup::Testclass, because that missing super is a deliberate strategy for that class.Everything runs green on my fork.