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

Add support for ignore pattern for LineLength cop #3479

Closed
swalkinshaw opened this issue Sep 8, 2016 · 2 comments
Closed

Add support for ignore pattern for LineLength cop #3479

swalkinshaw opened this issue Sep 8, 2016 · 2 comments

Comments

@swalkinshaw
Copy link

👋 We generally like and enforce the LineLength cop, but there are certain contexts where it's not ideal.

Proposal

My proposal is an IgnoredPatterns option which is an array of regexps. If any of these match a line, the line length cop is skipped.

Metrics/LineLength:
  Enabled: true
  Max: 120
  IgnoredPatterns:
    - '^\s*test\s.*'

Alternatives

There's two existing ways to deal with this (that I know of):

  • comments to disable the cop
  • ignore an entire file

Our main use case is ignore line lengths for test/spec descriptions.

Examples:

def test_some_really_long_minitest_test_method_name
end

test 'some really long test description which exceeds length' do
end

It's not ideal to have comments around every test method/description, or to ignore the file entirely.

Solution

I've already made a custom cop that implements this here: https://gist.github.com/swalkinshaw/c540a59b4983b335149c2f6fde69e7b9

I'd be happy to implement this via a PR in the existing LineLength cop if it's wanted.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 9, 2016

I'm on the fence about this. Typically I just suggest people to allow longer lines in their tests. (you can have different config files in different folders)

I'm curious to hear about thoughts on the subject.

@swalkinshaw
Copy link
Author

Yeah I agree it's a slippery slope. Ideally we still want to enforce line length on the test bodies since it's just code and we want to keep it readable. Unfortunately there's just no nice/readable way to make test descriptions multi-line to play by the rules.

So we're left in a weird situation where there are a few legitimate contexts where longer lines should be allowed and they happen consistently enough.

I can only speak for Shopify, but this has come up multiple times with developers asking if there's a way to disable this rule for test/spec descriptions. We have an issue on our styleguide with 35 comments so far about this issue :)

In case this feature doesn't make it in, I have an alternative proposal. I'm perfectly happy making custom cops but right now it's harder since you can't inherit from an existing one.

Please correct me if I'm wrong, but the way all cops are tracked right now via inherited doesn't allow subclassing LineLength for example.

An alternate method to track class descendants might make this more flexible.

jonas054 added a commit to jonas054/rubocop that referenced this issue Nov 26, 2016
A list of regular expressions. The cop ignores lines matching
any of them. Empty list by default.
chrisroos added a commit to Crown-Commercial-Service/crown-marketplace that referenced this issue Nov 19, 2018
The `IgnoredPatterns` option appears to have been added for exactly this
reason. See PR 3479[1] for more info.

This is a pre-emptive change that allows us to use longer test names in
a subsequent commit.

[1]: rubocop/rubocop#3479

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants