Monorepo: eslint strict boolean expressions #2030
Conversation
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
from
502f85e to
cb08b48
Compare
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
2 times, most recently
from
6a29984 to
d00a320
Compare
gabrocheleau
added
PR state: WIP
type: enhancement
package: block
package: blockchain
package: tx
package: vm
package: common
package: monorepo
package: ethash
package: devp2p
package: client
package: mpt
package: util
package: rlp
target: master
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
8 times, most recently
from
30a257b to
5fc0d9c
Compare
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
from
1c6be76 to
3348602
Compare
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
3 times, most recently
from
4517466 to
5894a17
Compare
|
Ugh. What a PR. 😜 Great though! 🙏 ❤️ |
|
|
||
| type Falsy = false | '' | 0 | null | undefined | ||
|
|
||
| export function isFalsy(value: unknown): value is Falsy { |
There was a problem hiding this comment.
Can you add a very very explicit note here (and for isTruthy as well) that this is a function which should only temporarily be used and otherwise be replaced with the direct checks (as you mentioned as well), together with some overall context why this function exists?
I otherwise have the strong fear that this looks like as something "how we do things" and other developers rather align with this.
There was a problem hiding this comment.
Good point. I added a note and marked them as deprecated. The goal should definitely to get rid of those eventually, and they should not be used in new code (I think the @deprecated natspec will help achieve that)
There was a problem hiding this comment.
Thanks, that's a good solution! 👍 😄
|
|
||
| export function isTruthy<T>(value: T | Falsy): value is T { | ||
| return !isFalsy(value) | ||
| } |
There was a problem hiding this comment.
Also, on another note: are you confident that this code is correct, since we would be using this all over the place now in our libraries (for now)? So does this match 1:1 the previous behavior?
I guess we very much would want to have some tests (or rather: somewhat complete, type-wise) on this, to be really sure this behaves correctly in all cases.
There was a problem hiding this comment.
Yeah I am confident, but can definitely add tests since as you say this is used throughout
There was a problem hiding this comment.
Thanks, yeah, guess this should be some really compact LoCs to cover the various cases.
There was a problem hiding this comment.
Added some tests
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
from
5894a17 to
38e5bf9
Compare
gabrocheleau
force-pushed
the
monorepo/eslint-strict-boolean-expressions
branch
from
38e5bf9 to
c9053dc
Compare
|
Ah, interestingly enough it is not working here (GitPOAP), but activated here #2035. Two theories:
We'll see (or someone has already the answer). 🙂 |
This PR adds the strict-boolean-expressions eslint rule and applies it throughout the monorepo. I also added
isTruthyandisFalsytype guard utils for convenience. Eventually, I think we can refactor most of thoseisTruthyandisFalsyby providing more accurate types upstream and explicitly checking for truthy/falsy cases instead of relying on JS truthiness.Related to #1935