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

Enable support for cops introduced by 0.85 #68

Merged
merged 6 commits into from Jul 8, 2020
Merged

Conversation

ivgiuliani
Copy link
Contributor

@ivgiuliani ivgiuliani commented Jun 10, 2020

Namely:

  • Lint/MixedRegexpCaptureTypes
  • Style/RedundantRegexpCharacterClass
  • Style/RedundantRegexpEscape
  • Style/AccessorGrouping
  • Style/BisectedAttrAccessor
  • Style/RedundantAssignment
  • Style/RedundantFetchBlock

rubocop.yml Show resolved Hide resolved
Namely:

- `Lint/MixedRegexpCaptureTypes`
- `Style/RedundantRegexpCharacterClass` (disabled by default due to
  rubocop/rubocop#8098)
- `Style/RedundantRegexpEscape`
@nickcampbell18
Copy link
Contributor

Would be also good to include the new cops from https://github.com/gocardless/payments-service/pull/25348

CHANGELOG.md Outdated
@@ -1,6 +1,14 @@
Changelog
=========

2.15.0
------
* Support new cop introduced by rubocop v0.87: `Style/AccessorGrouping`, `Style/BisectedAttrAccessor`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we feel about Style/AccessorGrouping? It groups blocks of attr_reader/writer together (see https://docs.rubocop.org/rubocop/cops_style.html#styleaccessorgrouping), but in cases where we've got a lot of them it may be better to split them logically

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's an element of personal style that I'd rather avoid forcing, e.g. this should be fine:

class Foo
  attr_reader :public_attr

  private

  attr_reader :private_attr
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly here myself; if this would cause lots of churn for us then it's probably not worth it. If it only affects a few places then might be worth turning it on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I think I'll disable it for now, tried it in PS and I'm not convinced the proposed changes would make it any better

Copy link
Contributor

@matt-thomson matt-thomson left a comment

Choose a reason for hiding this comment

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

👍

@ivgiuliani ivgiuliani merged commit 5a92941 into master Jul 8, 2020
@ivgiuliani ivgiuliani deleted the cop-0.85.1 branch July 8, 2020 14:59
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.

None yet

3 participants