-
Notifications
You must be signed in to change notification settings - Fork 26.6k
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
Added max-len rule #510
base: master
Are you sure you want to change the base?
Added max-len rule #510
Conversation
I think according to https://github.com/airbnb/javascript#6.2 our max-length is 100 characters, not 80 characters. Please make the length explicit. I thought that this number was already well-declared and obvious in our styleguide, but I think I was thinking of our Ruby styleguide (length guidance here: https://github.com/airbnb/ruby#line-length). Sorry to change direction so fast; I think we should wait for more discussion on the topic before merging this. |
Ok. I am fine with 100 characters as well. Should I modify this pull request or should we wait for others to discuss this whole thing? |
|
||
```javascript | ||
// bad | ||
var foo = { "bar": "This is a bar.", "baz": { "qux": "This is a qux" }, "difficult": "to read" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't see a rule in the styleguide about it, in our examples we generally don't quote object keys when they're not invalid identifiers. Can you unquote all of these object keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should submit a PR adding this rule :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And yes: if so then there should be a rule for that 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead and make it explicitly 100 chars in your markdown update and in the eslintrc. We'll let the PR sit here for a day or two after that and then I'll merge after consensus/people forget |
Done 😎 |
|
||
```javascript | ||
// bad | ||
var foo = { bar: "This is a bar.", baz: { "qux": "This is a qux" }, difficult: "to read" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"qux" is unnecessarily quoted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line should be obviously too long, as it is it's 96 characters, make it like 120+ chars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I am sorry. This happens when you are in hurry 😕. I will fix that
Should I take the description really in the whitespace section? I think it has nothing to do with whitespaces. |
@@ -83,6 +83,7 @@ module.exports = { | |||
}], | |||
'eqeqeq': 2, // http://eslint.org/docs/rules/eqeqeq | |||
'guard-for-in': 2, // http://eslint.org/docs/rules/guard-for-in | |||
'max-len': [2, 100, 4] // http://eslint.org/docs/rules/max-len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, max-len
is already specified in legacy.js
+1 I would like to see this merged. |
oops |
@screendriver if you'd like to rebase this on top of master, we can now add and renumber sections without breaking existing links. |
Closes #509