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.

Sign up for GitHub

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

Jump to bottom

Remove the code style rule about casts to bool #26220

Closed
wants to merge 2 commits into from
Closed

Remove the code style rule about casts to bool #26220

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67401ee
Remove the code style rule about casts to bool
richvdh Sep 22, 2023
f4749b8
fix numbering
richvdh Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions code_style.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,6 @@ Unless otherwise specified, the following applies to all code:
}
```

14. Explicitly cast to a boolean, rather than relying on implicit truthiness of non-boolean values:

```typescript
const isRealUser = !!userId && ...;
// ... or ...
const isRealUser = Boolean(userId) && ...;

// 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

15. Use `switch` statements when checking against more than a few enum-like values.
16. Use `const` for constants, `let` for mutability.
17. Describe types exhaustively (ensure noImplictAny would pass).
richvdh marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading