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.

Sign up for GitHub

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

Jump to bottom

Docs: fix wrong config in max-len example. #9309

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Docs: fix wrong config in max-len example. #9309

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e0f5ef
Docs: fix wrong config in max-len example.
aladdin-add Sep 14, 2017
4ec08ee
Update max-len.md
aladdin-add Sep 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/rules/max-len.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ var longRegExpLiteral = /this is a really really really really really long regul
Examples of **correct** code for this rule with the `{ "ignorePattern": true }` option:
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is also incorrect: ignorePattern is a string option, so ignorePattern: true doesn't make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe it make more sense to allow string and regex -- using RegExp(string) can be confusing, and sometimes different behavior. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think there are multiple issues we're discussing in this PR -- maybe we should separate them:

  • The current example in the documentation doesn't work, so I think we should fix it in this PR.
  • It seems like the comment parsing for config comments might have confusing behavior with backslashes. (Maybe we should create a new issue for this.)
  • It might be simpler for the rule to have a regex option rather than a string. (I think it's unlikely that this will work because we need to support JSON config files which don't support regex. However, feel free to create a new issue for it if you'd like.)

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. updated the PR, so it will not block the merge.:)


```js
/*eslint max-len: ["error", { "ignorePattern": "^\\s*var\\s.+=\\s*require\\s*\\(/" }]*/
/*eslint max-len: ["error", { "ignorePattern": "^\\s*var\\s.+=\\s*require\\s*\(" }]*/

var dep = require('really/really/really/really/really/really/really/really/long/module');
```
Expand Down