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

Curly all rule #237

Open
flippidippi opened this issue Jun 14, 2022 · 6 comments
Open

Curly all rule #237

flippidippi opened this issue Jun 14, 2022 · 6 comments

Comments

@flippidippi
Copy link
Contributor

flippidippi commented Jun 14, 2022

What version of this package are you using?
17.0.0

What problem do you want to solve?
Have a tighter rule around curly for better consistency.
Currently, the rule is set to "multi-line", which can still end up with code that isn't consistent or harder to read.

For example of consistency, both of these are ok and I think it's better to have a tighter rule around this.

if (foo) foo++
if (foo) {
  foo++
}

Another example if when the if statement is long, and the reader has to scan far to understand what will be done (usually this would happen with longer variables names)

if (foo != bar1 && foo != bar2 && foo != bar3 && foo != bar4 && foo != bar5 && foo != bar6 && foo != bar7) foo++

What do you think is the correct solution to this problem?
Change rule to curly: "all"

Are you willing to submit a pull request to implement this change?
Yes

@theoludwig
Copy link
Contributor

I agree with this rule, it adds consistency! 👍

@voxpelli
Copy link
Member

I'm not so sure about this one. It's stricter for sure, but standard doesn't aim to be the strictest of them all as I see it and I'm not sure it actually will always improve the code quality. I like the one liners at times.

@flippidippi
Copy link
Contributor Author

I think it can also improve readability as the reader's eyes always know where to look for the "what happens if" piece. I don't think it is just about being more strict, but improving predictability when reading other devs' code and reducing PR review time.

We can change this ourselves since we aren't using standard directly, but I thought this rule could help more teams than just ours.

@voxpelli
Copy link
Member

I think some initial checks in a function can be more readable as one liners, as it puts emphasis on the if-statements rather than the return value

const foo = (value) => {
  if (value === undefined) return 'foo';
  if (typeof value === 'string') return false;
  if (typeof value !== 'number') throw new TypeError('Wrong input');
  // ...
};

vs

const foo = (value) => {
  if (value === undefined) {
    return 'foo';
  }
  if (typeof value === 'string') {
    return false;
  }
  if (typeof value !== 'number') {
    throw new TypeError('Wrong input');
  }
  // ...
};

But when it comes to changes like this its also a matter of how breaking it would be to the wider community. We don't want to ship a breaking change without helping projects onboard that change.

@flippidippi
Copy link
Contributor Author

I can't believe you put semicolons in there. Are you even using standard? 😝

Yeah, I can def see those situations might be a good place for them as long as the if statements are shorter.
I do wonder how much it would break the wider community and if the --fix option would handle that.

@voxpelli
Copy link
Member

@flippidippi I'm using semistandard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants