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

Config proposals #33

Merged
merged 8 commits into from
Feb 13, 2023
Merged

Config proposals #33

merged 8 commits into from
Feb 13, 2023

Conversation

davidrapson
Copy link
Contributor

@davidrapson davidrapson commented Oct 24, 2022

Consider this a set of proposals for a v10 of the gem. Mostly proposing one new set of rules and updating some of the related config to get new versions and to include rubocop-rails in the gemspec.

Count arrays and hashes as one-line

Count arrays and hashes as one-line when calculating method and class length. We have these set in many of our internal rubocop configs so it might make sense to consider this a default preference.

Add rubocop-rails as as dependency and bump dependency versions

As far as I know there's no reason this can't be added as a dependency to avoid the extra step in the installation instructions. Bump all other rubocop dependencies to newer versions.

Add TargetRubyVersion to internal rubocop config

Avoids misleading comment in gemspec and reflects minimally supported rubocop version

Remove ruby-version file

This is a shared gem that is intended to run on any version of ruby 2.7 and upwards. Including a ruby-version file seems counter to the conventions of similar gems so perhaps we should remove this.

Avoids misleading comment in gemspec and reflects minimally supported
rubocop version.
Count arrays and hashes as one-line when calculating method and class
length. We have these set in many of our internal rubocop configs so it
makes sense to consider this a default preference.
As far as I know there's no reason this can't be added as a
dependency to avoid the extra step in the installation instructions.
This is a shared gem that is intended to run on any version of ruby 2.7
and upwards. Including a ruby-version file seems counter to the
conventions of similar gems so perhaps we should remove this.
@davidrapson davidrapson marked this pull request as ready for review October 24, 2022 08:28
default.yml Show resolved Hide resolved
default.yml Outdated Show resolved Hide resolved
Remove `spec/` exclude for `Metrics/BlockLength` as this is now a default in rubocop-rspec 2.11+
@@ -23,6 +22,7 @@ Gem::Specification.new do |spec|

spec.require_paths = ["lib"]

spec.add_dependency "rubocop", "~> 1.30.1"
spec.add_dependency "rubocop-rspec", "~> 2.11.1"
spec.add_dependency "rubocop", "~> 1.36"
Copy link
Contributor

Choose a reason for hiding this comment

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

It is up to 1.43 now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst I'm not here anymore, rubocop is quite lax on versioning. So using a minor lock might give you more piece of mind. Most ruby projects do to cover themselves unless new cops come in and bust CI runs

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we could loosen this up so we don't need a new version of this project for every rubocop update. A major lock might be sufficient.

The lock file means it won't randomly change on the consuming project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure about the major constraint, a constraint of ~> 1 is too loose. By design this is a set of conventional config. There will be options in our defaults that require a minimum version floor to be able to use them. I think the kind of pessimistic constraints we have now (at least y 1.x minor version, but not version 2) is what we want. We are only saying "at least" with these constraints so we don't need to update for every version unless we were outright pinning.

Copy link
Contributor

@luke-hill luke-hill Jan 16, 2023

Choose a reason for hiding this comment

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

FYI, ~> 1 is actually a bit erroneous. As such whilst it is permitted in the twiddle setting, I dislike it in all my projects and in cucumber-rails recently I got people to clean it up to the more verbose ~>1.x

For reference: ~> 1 is actually identical to >=1, < 2 (And as such it's identical to ~> 1.0 (... almost), But yeh it's maybe an issue with how ruby parses it. Hardly anyone uses it though

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out I didn't understand the twiddle. Thanks Luke. So ~> 1.43 would actually do what I want. Good change!

Copy link
Contributor

@mrdaniellewis mrdaniellewis left a comment

Choose a reason for hiding this comment

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

Works great, I'm going to merge this in

@mrdaniellewis mrdaniellewis merged commit 1107937 into main Feb 13, 2023
@mrdaniellewis mrdaniellewis deleted the config-proposals branch February 16, 2023 13:03
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