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.

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

chore: require brackets for conditional statements #49

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

scottdover
Copy link
Contributor

@scottdover scottdover commented Dec 6, 2022

Summary
This adds the curly rule to enforce consistency around conditional statements.

Testing

  • Made sure yarn lint && yarn format succeeded

@scottdover scottdover linked an issue Dec 7, 2022 that may be closed by this pull request
@2TomLi 2TomLi requested a review from scnwwu December 8, 2022 19:25
@2TomLi 2TomLi added the code quality Code quality related issues label Dec 8, 2022
@@ -13,5 +13,6 @@ module.exports = {
rules: {
eqeqeq: "error",
"prefer-const": "error",
curly: "error",
Copy link
Member

Choose a reason for hiding this comment

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

I would vote for ["error", "multi-line", "consistent"]. Some simple short lines look straightforward to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to lean towards explicit brackets for conditionals, for a couple reasons:

It provides consistency everywhere, where you don't have to think about how to write an if statement (an if statement will always have opening and closing brackets)

It provides a somewhat cleaner git history. For example, if I write code like...

if (someCheck) return value;

and you update the code to...

if (someCheck) {
  const calculatedValue = value + 1;
  return calculatedValue;
}

then it appears that you were responsible for defining someCheck (because you edited that line of code). So if a future dev has a question about someCheck, they might be more likely to reach out to you, although I was the one responsible for the original code. It's a super nitpicky edge case, though.

Also, I just find something like this kind of gross (but valid with w/ ["error", "multi-line", "consistent"]):

let sum = 0;
while (item.next()) sum += item.value;

I'd love to get other people's feedback on this one. I could definitely be over thinking it 😁

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I agree with you now:) Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @scnwwu. Looks good here

@scottdover scottdover marked this pull request as ready for review December 12, 2022 14:13
@scottdover scottdover changed the title Update eslint to address inconsistencies chore: require brackets for conditional statements Dec 12, 2022
@scottdover scottdover merged commit 1bf0c87 into main Dec 12, 2022
@scottdover scottdover deleted the chore/eslint-changes branch December 12, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Code quality related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix inconsistent code formatting
4 participants