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

Support ruby 3.1 #189

Merged
merged 14 commits into from
Feb 10, 2023
Merged

Support ruby 3.1 #189

merged 14 commits into from
Feb 10, 2023

Conversation

RenzoMinelli
Copy link
Contributor

This PR is based on #178 (Author: Brendan Mulholland), these changes are required for this gem to support Ruby 3.1:

  • Update rubocop to 1.22.x
  • Update rubocop-rspec to 2.0.x


inspect_source(source)
expect(cop.offenses.size).to eq(1)
it 'rejects constant defined in global space' do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test's description was changed to reflect what it is being tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good correction

'end',
].join("\n")

xit 'rejects with "<<" static methods and a non-matching name' do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was set as pending due to the fact that a bug is causing it to fail, suggesting baz to be defined in foo/foo.rb instead of being in foo.rb.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange to me. Why is the metaclass of Foo expected to be defined in foo/foo.rb? Was this failing previously? I'm assuming this is due to the rubocop upgrade?

I'd like to dig into this one and figure out why this changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I should have left the link to the issue that I created discussing this. It's a false positive in master.

Copy link
Contributor

@pariser pariser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor questions – thanks for the contibution!

@@ -56,7 +56,6 @@ RSpec/DescribedClass:
RSpec/EmptyExampleGroup:
Description: Checks if an example group does not include any tests.
Enabled: true
CustomIncludeMethods: []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious the justification for removing this? is it that the default value is equivalent to empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upgrade guide to version 2.x of rubocop-rspec indicates that the CustomIncludeMethods configuration option for RSpec/EmptyExampleGroup was removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I must have pulled up an old reference to the EmptyExampleGroup docs. Thanks for the reference.

'end',
].join("\n")

xit 'rejects with "<<" static methods and a non-matching name' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange to me. Why is the metaclass of Foo expected to be defined in foo/foo.rb? Was this failing previously? I'm assuming this is due to the rubocop upgrade?

I'd like to dig into this one and figure out why this changed


inspect_source(source)
expect(cop.offenses.size).to eq(1)
it 'rejects constant defined in global space' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good correction

@@ -13,6 +13,8 @@ module SpecHelper
Dir.glob(spec_helper_glob).map(&method(:require))

RSpec.configure do |config|
config.include RuboCop::RSpec::ExpectOffense
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for switching to ExpectOffense syntax!

@@ -25,9 +25,9 @@ Gem::Specification.new do |spec|
'Gemfile',
]

spec.add_dependency('rubocop', '~> 0.93.1')
spec.add_dependency('rubocop', '~> 1.22.0')
Copy link
Contributor Author

@RenzoMinelli RenzoMinelli Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to bump the version to grater than 1.22.0 because if I tried running rubocop with any version between 1.0.0 and 1.21.0 errors would occur, displaying messages like this:

An error occurred while Layout/BlockAlignment cop was inspecting ~/ruby/rubocop-airbnb/spec/rubocop/cop/airbnb/unsafe_yaml_marshal_spec.rb:1:0

Ending with:

Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
https://github.com/rubocop/rubocop/issues

@pariser
Copy link
Contributor

pariser commented Feb 9, 2023

@RenzoMinelli Thanks! This is great and I'm pretty sure it's ready to go, but I'd love to get tests passing on this branch before we merge. I just merged #191 which switched out travis for github actions (apparently Travis hadn't been working for quite a long time). Would you mind rebasing? You'll have to add ruby 3.1 (and uncomment 3.0) in the CI matrix here.

@RenzoMinelli
Copy link
Contributor Author

@RenzoMinelli Thanks! This is great and I'm pretty sure it's ready to go, but I'd love to get tests passing on this branch before we merge. I just merged #191 which switched out travis for github actions (apparently Travis hadn't been working for quite a long time). Would you mind rebasing? You'll have to add ruby 3.1 (and uncomment 3.0) in the CI matrix here.

I just rebased my branch, but it seems that First-time contributors need a maintainer to approve running workflows

@pariser
Copy link
Contributor

pariser commented Feb 10, 2023

Approved your workflow! Tests are failing in 2.4 due to rubocop upgrade. That's a good tradeoff (support 3.1, drop support for 2.4). Feel free to update the matrix, and bump the required_ruby_version (link).

@RenzoMinelli
Copy link
Contributor Author

@pariser seems that an approval in needed for each workflow 😅. I removed 2.4 in the last commit.

@pariser
Copy link
Contributor

pariser commented Feb 10, 2023

So it seems! I'll unlock CI right now

Copy link
Contributor

@pariser pariser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll merge this and cut a new (major) release today.

@RenzoMinelli
Copy link
Contributor Author

Thank you! I'll merge this and cut a new (major) release today.

Nice! Thanks

@pariser pariser merged commit 762cd62 into airbnb:master Feb 10, 2023
@pariser pariser mentioned this pull request Sep 21, 2023
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.

2 participants