Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Add jscs rule requireCurlyBraces and disallowKeywordsOnNewLine #10865

Closed
wants to merge 4 commits into from

Conversation

rzschech
Copy link
Contributor

I split the requireCurlyBraces changes into three commits to make it easer to review the code. They are also ordered from the least contentious (adding curly braces to statements that are already multiline) to most contentious (single line statements that return, break or throw) which makes the code more verbose.

The disallowKeywordsOnNewLine changes are unfortunately dependent on the requireCurlyBraces so can't be done as an independent pull request.

This is in preperation for adding the jscs rule "requireCurlyBraces"
…containing return, break, or throw

This is in preperation for adding the jscs rule "requireCurlyBraces"
Add curly braces to single line control flow statements
@caitp
Copy link
Contributor

caitp commented Jan 26, 2015

i think we should stop adding new style rules to enforce :| this is getting too restrictive

@lgalfaso
Copy link
Contributor

I am ok with these changes, but I am ok with what the majority thinks is best

@hzoo
Copy link
Contributor

hzoo commented Jan 27, 2015

I was thinking maybe we remove the remaining .jscs.json.todo file since those rules were only initially there as suggestions. Maybe even remove rules if a certain rule keeps being committed in PRs?

Although they should be for the most part pretty good - I tried opposite rules to see how much more percentage a style was being used.

@petebacondarwin
Copy link
Member

I would be quite keen to clean out the cases where we have an if with a line break but no curly braces

But we generally accept in our code base that a simple if statement can have a single statement that appears on the same single line as the if. Since there is no rule for this specific scenario that doesn't also prevent the single line scenario above, I don't think we can include the requireCurlyBraces rule.

And so I don't think we can land this PR. Sorry @rzschech. Thank you for looking into it.

@hzoo
Copy link
Contributor

hzoo commented Feb 2, 2015

There is an issue for an exception to allow single statements (like returns) so maybe in the future jscs-dev/node-jscs#244

@petebacondarwin
Copy link
Member

@hzoo - yes, I think we are about there on the JSCS rules for now.

@petebacondarwin
Copy link
Member

@rzschech - I did land the first commit though, since I like that style even if it is not enforceable right now.

@rzschech
Copy link
Contributor Author

rzschech commented Feb 2, 2015

@petebacondarwin thanks, I thought this might be contentious hence splitting the pull request into a few commits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants