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

Bump minimum RuboCop version #210

Closed
wants to merge 2 commits into from
Closed

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Mar 24, 2024

This PR originally just added the minimum RuboCop version to the CI matrix, but it was discovered that that version doesn't work, so in now also bumps it.

This bumps the minimum RuboCop version to 1.45.1, to reflect actual compatibility.

It also adds the minimum RuboCop version to the CI matrix. This ensures we're compatible with the minimum RuboCop version we claim in rubocop-sorbet.gemspec.

This approach is copied from rubocop-shopify.

@sambostock sambostock requested a review from a team as a code owner March 24, 2024 18:33
@sambostock
Copy link
Contributor Author

Actually testing against RuboCop 0.90.0 reveals that we don't support it (despite our gemspec saying we do).

#207 doesn't work as RuboCop 1.6 was the first version to define RuboCop::ConfigObsoletion.files, which we attempt to write to. I looked into backporting this, but 0.90.0 is so old it would be a huge change.

However, using RuboCop 1.6.0 still has over 100 test failures. I ran the test suite against every RuboCop version, and the point where it crosses over into compatibility is around RuboCop 1.45.1.

RuboCop Version Tests
< 1.43.0 ❌ Fail
= 1.43.0 ❌ Fail
= 1.44.0 ✅ Pass
= 1.44.1 ✅ Pass
= 1.45.0 ❌ Fail
= 1.45.1 ✅ Pass
= 1.46.0 ✅ Pass
> 1.46.0 ✅ Pass

Based on that, I propose that we change the minimum RuboCop version to reflect actual compatibility (rather than try to backport compatibility).

-spec.add_runtime_dependency("rubocop", ">= 0.90.0")
+spec.add_runtime_dependency("rubocop", ">= 1.45.1")

@@ -11,15 +11,20 @@ jobs:
strategy:
fail-fast: false
matrix:
ruby: ["2.7", "3.0", "3.1", "3.2"]
name: Test Ruby ${{ matrix.ruby }}
ruby: ["2.7", "3.0", "3.1", "3.2", "3.3"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop 2.7 since it's EOL?

@@ -33,7 +38,7 @@ jobs:
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 3.2
ruby-version: 3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but we should be able to switch to the 'centralized' approach, similiar to https://github.com/Shopify/ruby-lsp/pull/1830/files

@sambostock
Copy link
Contributor Author

@andyw8 I'll address both your Ruby version related comments in a separate PR 👍

@sambostock sambostock changed the title Add minimum RuboCop version to CI matrix Bump minimum RuboCop version Mar 26, 2024
This ensures we're compatible with the minimum RuboCop version we claim
in `rubocop-sorbet.gemspec`.

This approach is copied from `rubocop-shopify`.
This change reflects the actual current compatibility of the gem, as
versions prior to this one do not work.
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 26, 2024
@github-actions github-actions bot closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants