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

Option to ignore max-len based on a regexp #2934

Closed
bgw opened this issue Jul 6, 2015 · 13 comments

Comments

Projects
None yet
6 participants
@bgw
Copy link
Contributor

commented Jul 6, 2015

There's outstanding suggestions to make exceptions for comments (#2221) and URI comments (#1661), but I personally think a better solution is to allow arbitrary regexps, at the cost of being less robust for those above cases. This would allow other types of exceptions, such as for really long require lines.

If we were to implement this, one issue I see is we currently use context.toSourceLines(). Should we restrict regexps to a single line, or should we also allow multiline matches (probably with an associated performance hit)?

I'd be happy to make a PR if accepted.

@gyandeeps gyandeeps added the triage label Jul 7, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

Can you explain a use case other than URLs and comments?

Keep in mind, URL regexes are long and complicated, so I don't think we can ask people to manually configure that.

@bgw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

The reason why we want it at Facebook is because our old linting system had a way to ignore really long require statements. Because we have some long module names, and because we wrap at 80 columns, if you had a long module name, you'd end up with:

// TODO: Implement enterprise fizzbuzz adapter
var FooBarAbstractIntrospectingFactoryConstructorInterfaceDerivative = require('FooBarAbstractIntrospectingFactoryConstructorInterfaceDerivative');

I agree URL regexes are difficult. We could take an object as an argument to max-len, which could have keys for special cases like comments and URLs:

rules:
  # ignore urls and require statements, but not normal (non-URL) comments
  max-len: [2, 80, 4, {urls: true, comments: false, regexp: '^\s*var\s.+=\s*require\s*\('}]
@mathieumg

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2015

I think @bgw's suggested configuration is very sensible! Flexible (allows for more advanced use cases) and easy to configure for the common scenarios.

@dozoisch

This comment has been minimized.

Copy link

commented Jul 7, 2015

👍 love that last config option. Especially useful for import/requires!

@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

  1. I think a regexp would be very error-prone for this rule. Consider the following example:

    var code = `var foo = "bar";
    var bar = require(
    `;
  2. Why should this be limited to a single pattern?

@dozoisch

This comment has been minimized.

Copy link

commented Jul 7, 2015

To some extent you can have more than one pattern in a regex /(pattern1)|(pattern2)/ and I mean at some point, these rules are there to help you pick code style issues. I think the current proposed solution cover most uses cases.

@bgw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

@lo1tuma, Are you referring to the fact that the match would match multiple lines? That regexp was assuming matches were constrained to a single line, based on our use of context.toSourceLines().

If you're referring to the unclosed parenthesis, that'd be caught by a parse error.

That said, I agree regexps can't match everything. The advantage of using them here is that we could support arbitrary use-cases, rather than constraining ourselves to some short list of special-cases. Plus, if a regexp matches something it shouldn't, only that one line gets ignored, which isn't the end of the world. IMHO, it's better for the linter not to report something than to report things it shouldn't.

Another syntax to consider would be to accept an arbitrary number of additional positional arguments, and use a string prefix to mark regexp rules, eg:

rules:
  # ...
  max-len: [2, 80, 4, 'ignore-urls', 'ignore-regexp:^\s*var\s.+=\s*require\s*\(']
@bgw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

Oh, another problem, I just noticed: If this is YAML or JSON, those regexps actually need to be double-escaped, which really mucks up the syntax. (whoops!)

@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

@bgw No, my point is that the regexp would also match the content of string literal (or in my example above a template literal).

@bgw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

@lo1tuma, I see. I'll stand by my second point in that case:

if a regexp matches something it shouldn't, only that one line gets ignored, which isn't the end of the world. IMHO, it's better for the linter not to report something than to report things it shouldn't.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

OK, seems reasonable as an addition to the two other opt-outs. Id suggest the following:

"max-len": [ 2, 80, 4, {
   ignorePattern: " foo"
}]

@bgw would you be willing to submit a PR for this?

@nzakas nzakas added enhancement rule accepted and removed triage labels Jul 8, 2015

@bgw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2015

@nzakas, Sure. I'll start working on it sometime today. Do we want to handle multi-line regexp, or stick to single-line matches only?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

I'd start with single line for now.

bgw added a commit to bgw/eslint that referenced this issue Jul 13, 2015

New: Add ignorePattern, ignoreComments, and ignoreUrls options to max…
…-len (fixes eslint#2934, fixes eslint#2221, fixes eslint#1661)

The code is a bit messy, but heavily commented and unit testing is
included to try to capture all of the resulting edge-cases.

This breaks eslint-tester because of eslint/eslint-tester#28 and
eslint/eslint-tester#24.

bgw added a commit to bgw/eslint that referenced this issue Jul 14, 2015

New: Add ignorePattern, ignoreComments, and ignoreUrls options to max…
…-len (fixes eslint#2934, fixes eslint#2221, fixes eslint#1661)

The code is a bit messy, but heavily commented and unit testing is
included to try to capture all of the resulting edge-cases.

This breaks eslint-tester because of eslint/eslint-tester#28 and
eslint/eslint-tester#24.

@nzakas nzakas closed this in a5bef02 Jul 23, 2015

nzakas added a commit that referenced this issue Jul 23, 2015

Merge pull request #2986 from bgw/max-len-enhancements
New: Add ignorePattern, ignoreComments, and ignoreUrls options to max-len (fixes #2934, fixes #2221, fixes #1661)

IanVS added a commit that referenced this issue Jul 29, 2015

New: Add ignorePattern, ignoreComments, and ignoreUrls options to max…
…-len (fixes #2934, fixes #2221, fixes #1661)

The code is a bit messy, but heavily commented and unit testing is
included to try to capture all of the resulting edge-cases.

This breaks eslint-tester because of eslint/eslint-tester#28 and
eslint/eslint-tester#24.

IanVS added a commit that referenced this issue Jul 29, 2015

New: Add ignorePattern, ignoreComments, and ignoreUrls options to max…
…-len (fixes #2934, fixes #2221, fixes #1661)

The code is a bit messy, but heavily commented and unit testing is
included to try to capture all of the resulting edge-cases.

This breaks eslint-tester because of eslint/eslint-tester#28 and
eslint/eslint-tester#24.

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.