Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Eslint rule for block curlies #3955

Merged
merged 3 commits into from
Dec 23, 2016
Merged

Eslint rule for block curlies #3955

merged 3 commits into from
Dec 23, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 22, 2016

  • Add curly rule to disallow single-line squashing with blocks, e.g. ifs
  • Fix existing issues where we tripped the the new rule

Probably controversial, but has some good reasons and intent behind it -

  • Codebase is consistent, no more stylistic/preference differences
  • Codebase is fully clear as to intent and readable (leaving the squashing to Uglify)
  • Coverage tools can properly report on statements hit, important as we expand in this area (single line if-then would show as covered even if just the condition is tested and then never was)
  • Less chatter in PRs

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 22, 2016
@derhuerst
Copy link
Contributor

derhuerst commented Dec 22, 2016

Coverage tools can properly report on statements hit, important as we expand in this area (single line if-then would show as covered even if just the condition is tested and then never was)

Note that this is not true. Coverage tools that report on line hits will be fixed. Statement hits, which are more helpful, have worked before.

Still I support this because it makes the code more consistent and even line coverage is a useful metric.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 86.503% when pulling 921b9d0 on js-eslint-consistency into 077069c on master.

@derhuerst derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 23, 2016
@gavofyork gavofyork merged commit 714298a into master Dec 23, 2016
@gavofyork gavofyork deleted the js-eslint-consistency branch December 23, 2016 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants