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

Remove the code style rule about casts to bool #26220

Closed
wants to merge 2 commits into from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Sep 22, 2023

For discussion, but:

The rule as currently written appears to serve no value, and is honoured more in the breach than the observance in any case.

The current rules lack consistency (you are allowed to cast sometimes but not others?).

It might be an option to ban casts to boolean altogether (thus meaning you don't need to remember whether an empty object / empty array / empty string / etc is truthy), but that prevents a bunch of ergonomic conventions which we use heavily, so would be a bit heavy-handed.


This change is marked as an internal change (Task), so will not be included in the changelog.

@richvdh richvdh requested a review from a team as a code owner September 22, 2023 09:20
@richvdh richvdh added the T-Task Tasks for the team like planning label Sep 22, 2023
code_style.md Outdated Show resolved Hide resolved
// but *not*:
const isRealUser = userId && ...; // invalid implicit cast
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, reading this, I actually agree with it. If I am storing a boolean value in a variable I do want to be explicit about it, and probably also if I'm using && or || in an if condition.

But I would like to allow if (myObject) { at least for things that are MyClass | undefined or MyClass | null since I think it is clearer and easily understood.

Copy link
Member

Choose a reason for hiding this comment

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

I also find myself agreeing with the rule at least as given by the example here. Perhaps we just need to make this clearer & more specific, both in the explanation and with more examples? If anything, the problem with the example above is not that there is an implicit cast, it is that there no cast at all. That is to say, isRealUser will be a string object in memory (assuming that's what userId etc. is).

Maybe something like:

If a variable's type should be boolean, make sure it really is one.

const isRealUser = !!userId && ...; // good
const isRealUser = Boolean(userId) && ...; // also good
const isRealUser = userId && ...;   // bad: isRealUser is userId's type, not a boolean

if (userId) // fine: userId is evaluated for truthiness, not stored as a boolean

I've left out function arguments here: passing a non-boolean as a boolean would cause a type error (at least in the TS playground).

I don't think this is enforceable with eslint, sadly, but hopefully with a stricter definition we might stand a better chance of enforcing it socially?

Copy link
Contributor

Choose a reason for hiding this comment

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

I fully agree with @dbkr 's comment, and I think we somewhat already follow this, implying that @richvdh 's concern about how to enforce it if we can't write a rule might possibly not apply here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think I'd find @dbkr 's proposal acceptable.

const isRealUser = !!userId && ...; // good
const isRealUser = Boolean(userId) && ...; // also good

good provided what comes after the && is also a boolean, of course. Might also be worth including an example with ||.

Copy link
Member

Choose a reason for hiding this comment

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

Mm, good point. I'll paste an edited version rather than editing the version above, hopefully that's least confusing:

const isRealUser = !!userId && ...; // good
const isRealUser = Boolean(userId) && Boolean(userName); // also good
const isRealUser = Boolean(userId) && isReal; // also good (where isReal is another boolean variable)
const isRealUser = Boolean(userId && userName); // also fine
const isRealUser = Boolean(userId || userName); // good: same as &&
const isRealUser = userId && ...;   // bad: isRealUser is userId's type, not a boolean

if (userId) // fine: userId is evaluated for truthiness, not stored as a boolean

Copy link
Member Author

Choose a reason for hiding this comment

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

@dbkr do you fancy opening a new PR for that 😇 ? We could use this one but tbh it's a long way from the description.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay - done: #26317

@germain-gg germain-gg removed their request for review September 25, 2023 08:52
dbkr added a commit that referenced this pull request Oct 5, 2023
This is the proposal from the comments on #26220
(so effectively an alternative to that PR, if you like).
@richvdh
Copy link
Member Author

richvdh commented Oct 5, 2023

closing in favour of #26317

@richvdh richvdh closed this Oct 5, 2023
dbkr added a commit that referenced this pull request Oct 9, 2023
This is the proposal from the comments on #26220
(so effectively an alternative to that PR, if you like).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants