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

max-len to ignore Regular Expressions #3229

Closed
joezimjs opened this issue Jul 31, 2015 · 29 comments
Closed

max-len to ignore Regular Expressions #3229

joezimjs opened this issue Jul 31, 2015 · 29 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@joezimjs
Copy link
Contributor

Is there a way to add an option to ignore regular expressions that are too long? Sometimes those things can get huge. For example, URI.js needs a very long regex to parse a URL: https://github.com/medialize/URI.js/blob/gh-pages/src/URI.js#L207

@eslintbot
Copy link

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@ilyavolodin ilyavolodin added the triage An ESLint team member will look at this issue soon label Jul 31, 2015
@nzakas
Copy link
Member

nzakas commented Aug 1, 2015

See the ignorePattern option: http://eslint.org/docs/rules/max-len#options

@nzakas nzakas closed this as completed Aug 1, 2015
@bajtos
Copy link

bajtos commented Apr 1, 2016

@nzakas could you please share an example of the RegExp that describes a line containing regexp definition only, possibly with an assignment to a variable? I think the expression would be pretty complex and thus it may be better to implement his particular exception as a first-class eslint option.

Here is an example of what I would like to achieve:

// assuming max-len 4, these are considered valid:
var foo = /asd/;

foo(
  x,
  /asd/
);

// this is not valid - the comment should be on a different line
var foo = /asd/; /* trailing comment */

// this is not valid - function arguments should be split to multiple lines
foo(x, /asd/);

For example, jscs's maximumLineLength rule has an option regex, which allows regular expression literals to break the rule, see http://jscs.info/rule/maximumLineLength

Thoughts?

@markelog
Copy link
Member

It was quite popular option - to ignore regular expression, it was used in jquery preset and other derivates like grunt and wordpress.

In order to increase compatibility with jscs i would like to put this back to agenda and allow myself to reopen this ticket

@markelog markelog reopened this May 10, 2016
@markelog markelog added this to the JSCS Compatibility milestone May 10, 2016
@alberto alberto added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 15, 2016
@nzakas
Copy link
Member

nzakas commented May 15, 2016

@markelog can you explain your proposal?

@markelog
Copy link
Member

markelog commented May 16, 2016

Sure, regular expression could be very long - like so https://github.com/jquery/jquery/blob/95c7ab68970ce201a2bbff48c8e951d38c228ce8/src/manipulation.js#L32, divide them like new RegExp( '...' + '...' ) wouldn't be ideal but hacky and do the regexp for ignorePattern would much harder then to analyze the ast.

It would be cool if this rule would have an option which would allow to ignore such long regexes, same as maximumLineLength allExcept: ["regex"] option of JSCS

@nzakas
Copy link
Member

nzakas commented May 16, 2016

@markelog sorry, what I meant was, what is your proposal for the new rule option?

@markelog
Copy link
Member

markelog commented Jun 5, 2016

Sorry, missed your comment, how about ignoreRegex? Which would ignore lines with regexes

@nzakas
Copy link
Member

nzakas commented Jun 7, 2016

Sorry, what I meant was can you put together a proposal based on our guidelines (http://eslint.org/docs/developer-guide/contributing/rule-changes). This issue was closed because there wasn't enough info, so before we can move forward, we need a concrete proposal.

@markelog
Copy link
Member

markelog commented Jun 8, 2016

Sorry, forgot about this bit, you want me to create another issue or do it here?

@nzakas
Copy link
Member

nzakas commented Jun 14, 2016

You can do it here (in the future, though, opening a new issue would be better).

@Herst
Copy link

Herst commented Jun 21, 2016

So is there still need to make this proposal more concrete or is it valid as-is?

@nzakas
Copy link
Member

nzakas commented Jun 22, 2016

@Herst still waiting for a concrete proposal.

@markelog
Copy link
Member

Sorry for the delay -
For

"max-len": [
        "error",
        {
                "code": 5,
                "ignoreRegex": true
        }
]

Valid:

var re = /w+/;

Invalid:

var re = "test";

If there is something i can make more clear, would you mind pointing me to the example?

@nzakas
Copy link
Member

nzakas commented Jun 30, 2016

@markelog that looks good! We just needed something concrete to discuss. I'm assuming that you are championing this change?

@markelog
Copy link
Member

markelog commented Jul 1, 2016

@nzakas Totally, can we label this as accepted?

@ilyavolodin
Copy link
Member

👍

1 similar comment
@mysticatea
Copy link
Member

👍

@markelog markelog added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 9, 2016
@markelog markelog self-assigned this Jul 9, 2016
@markelog
Copy link
Member

markelog commented Jul 9, 2016

ok-ok, thanks, i'm on it :)

@chenzhenxi
Copy link

chenzhenxi commented Jul 18, 2016

I use ignorePattern for work around this.
hope this gonna helpful.
"ignorePattern": "^\\s*(const|let|var)\\s+\\w+\\s+\\=\\s+\\/.*\\/(|i|g|m|ig|im|gm|igm);?$",

this regex will match codes like
const a = /asdfadsjl/;
const b = /asdfadsjl/
let c = /asdfadsjl/g
var d = /asdfadsjl/gm;
etc...

@Herst
Copy link

Herst commented Jul 18, 2016

@chenzhenxi Shouldn't that be forward slashes?

@chenzhenxi
Copy link

@Herst good catch, I edited my comment.

@kaicataldo
Copy link
Member

@markelog Any progress on this? :)

@markelog
Copy link
Member

Yeah, slowly but surely

@NenadP
Copy link

NenadP commented Aug 30, 2016

Would appreciate clean solution. as a workaround I'm using:
...

// eslint-disable-next-line max-len
url: /(<my_very_long_regex>)/,

@nzakas
Copy link
Member

nzakas commented Aug 30, 2016

I'm locking this conversation because the work is underway and the approach has already been discussed.

@platinumazure
Copy link
Member

@markelog @nzakas Is anyone actively working on this?

@not-an-aardvark
Copy link
Member

This was fixed in 644d25b, not sure why the issue wasn't closed automatically.

@eslint eslint unlocked this conversation Nov 24, 2016
@not-an-aardvark
Copy link
Member

Since it looks like this issue was causing some confusion: The new option is ignoreRegExpLiterals, not ignoreRegex as mentioned above. (For more info on the new option, see the rule docs.)

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
No open projects
Development

No branches or pull requests