-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
adding curly brackets to get rid of lint errors #1156
Conversation
@emkay any statements you were specifically worried about / want to get a second look? The code overall looks good to me (+1) I'll merge this asap if you're comfortable, these changes get stale very quickly |
I'm confident, and tests passed locally. I wasn't so much worried about introducing bugs, but rather that I saw different styles thrown around and just stuck to my personal preference when unsure which one to use. |
@@ -7,8 +7,12 @@ var optional = require('./optional') | |||
|
|||
|
|||
exports.parse = function(str) { | |||
if (str && str.uri) str = str.uri | |||
if (typeof str !== 'string') throw new Error('The cookie function only accepts STRING as param') |
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.
If possible, I'd like to keep in the style guide that we can do all single line statement without curly braces.
I do hate them being used when you drop down a line though. If there isn't a way to do only one then I'm fine with these changes.
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.
I think we can set that in the .eslintrc file.
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.
@mikeal it looks like the rule is not flexible enough to do that.
Personally I'm in favor of always using braces because:
- the code reads easier that way (1-1 correspondence between conditional and indentation)
- it's pretty common that you need to add more lines to an existing statement
For really short stuff I tend to use immediate if instead.
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.
+1, I'd like to require curly braces in all cases
👍 this is exactly how I would have done it. As a next step what do you guys think about changing this rule from a warning to an error? |
Merging this now so that it doesn't get stale, and we can spin the future rule / single-line if statement discussion into a seperate issue |
adding curly brackets to get rid of lint errors
@emkay awesome stuff! This makes our lint output actually readable/useful, thanks for taking the time to get this in |
adding curly brackets to get rid of lint errors
@FredKSchott this should fix the curly brace lint errors. I saw a mix of styles on where to put
else
andelse if
and if the curly brace was kept in on the same line as the body or not. I used my best judgement, but am happy to change if others have problems with either style. Just want to get rid of those lint errors. :)