style: enable all biomejs recommended rules #7153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Would be much easier to review commit by commit. |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Oct 11, 2024
| @@ -92,7 +92,7 @@ export function signedBlindedBlockToFull( | |||
|
|
|||
| // state transition can't seem to handle executionPayloadHeader presense in merge block | |||
| // so just delete the extra field we don't require | |||
| delete (signedBlock.message.body as {executionPayloadHeader?: ExecutionPayloadHeader}).executionPayloadHeader; | |||
| (signedBlock.message.body as {executionPayloadHeader?: ExecutionPayloadHeader}).executionPayloadHeader = undefined; | |||
There was a problem hiding this comment.
Not a fan of the performance/noDelete rule, setting a property to undefined is not equivalent in some cases
e.g.
> Object.keys({})
[]
> Object.keys({foo: undefined})
[ 'foo' ]or when merging objects
> {...{hello: "world"}, ...{}}
{ hello: 'world' }
> {...{hello: "world"}, ...{hello: undefined}}
{ hello: undefined }sometimes you just wanna get rid of the property altogether
There was a problem hiding this comment.
I'm actually a fan of this one. Not having delete makes turbofan optimization much better. We should be paying attention to the usage. I've noticed it when we go from one SSZ container to another and curious if there is a way to update the ssz to ignore undefined fields if they are not part of the container definition...
There was a problem hiding this comment.
ssz to ignore undefined fields if they are not part of the container definitio
that's already the case, it only reads fields that are defined in the container, and ignores all others
Not having delete makes turbofan optimization much better
need to carefully review all cases then that are in this PR, afterwards it should be fine, just don't like lint rules that change behavior
There was a problem hiding this comment.
Reverted the change for this rule from this PR.
I am in favor of not using delete, but this should be done with more deliberate review in a separate PR.
There was a problem hiding this comment.
Let's do it in another PR if this has performance improvements we can take advantage of
There was a problem hiding this comment.
this may have unintended consequences... i forget the exact situation but the ssz library was not happy about creating a root (or maybe it was container statics accepting the value, but i forget) when there was an undefined after converting container types...
nflaig
reviewed
Oct 12, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7153 +/- ##
============================================
+ Coverage 49.18% 49.21% +0.02%
============================================
Files 598 598
Lines 39801 39726 -75
Branches 2082 2092 +10
============================================
- Hits 19576 19550 -26
+ Misses 20184 20136 -48
+ Partials 41 40 -1 |
wemeetagain
reviewed
Oct 14, 2024
nflaig
reviewed
Oct 14, 2024
| @@ -636,45 +636,43 @@ export function getNotSeenValidatorsFn(state: CachedBeaconStateAllForks): GetNot | |||
| } | |||
|
|
|||
| // altair and future forks | |||
| else { | |||
There was a problem hiding this comment.
do we really think this is a good change? Especially for larger code block being explicit with the else seems good to me as it does not rely on if-block to return or throw
There was a problem hiding this comment.
Was suggested by @wemeetagain snd I am also in favor of not using such else bocks. More levels of indentations make code less readable.
There was a problem hiding this comment.
More levels of indentations make code less readable.
I don't think this can be applied generally, some cases I think identation can help make it more readable
There was a problem hiding this comment.
no strong opinions from me. I don't think that this needs to be enforced by the linter.
There was a problem hiding this comment.
I would just go with the style that the original author of the code preferred, else is more explicit and can support readability imo.
Doesn't apply everything e.g. sometimes you just wanna throw an error and not use else, if it's a 1-liner
There was a problem hiding this comment.
If indentation is needed for readability, one can use code blocks. There is a reason community named such style noUselessElse.
Also if there is larger code block which needs indentation for readability, it should be splited into multiple functions.
There was a problem hiding this comment.
I tend to agree with that... else is a code smell of overly long code blocks that could be made more readable...
packages/fork-choice/test/unit/protoArray/getCommonAncestor.test.ts
Outdated
Show resolved
Hide resolved
nflaig
previously approved these changes
Oct 14, 2024
matthewkeil
approved these changes
Oct 14, 2024
| @@ -92,7 +92,7 @@ export function signedBlindedBlockToFull( | |||
|
|
|||
| // state transition can't seem to handle executionPayloadHeader presense in merge block | |||
| // so just delete the extra field we don't require | |||
| delete (signedBlock.message.body as {executionPayloadHeader?: ExecutionPayloadHeader}).executionPayloadHeader; | |||
| (signedBlock.message.body as {executionPayloadHeader?: ExecutionPayloadHeader}).executionPayloadHeader = undefined; | |||
There was a problem hiding this comment.
this may have unintended consequences... i forget the exact situation but the ssz library was not happy about creating a root (or maybe it was container statics accepting the value, but i forget) when there was an undefined after converting container types...
wemeetagain
approved these changes
Oct 14, 2024
nflaig
approved these changes
Oct 14, 2024
philknows
pushed a commit
that referenced
this pull request
Oct 18, 2024
* Enable recomended correctness/noVoidTypeReturn * Enable recomended rule correctness/useYield * Enable recomended rule performance/noAccumulatingSpread * Enable recomended rule performance/noDelete * Enable recomended rule suspicious/noAsyncPromiseExecutor * Enable recommended rule suspicious/noDoubleEquals * Enable recommended rule suspicious/noDuplicateTestHooks * Enable recommended rule suspicious/noExportsInTest * Enable recommended rule suspicious/noFallthroughSwitchClause * Enable recommended rule suspicious/noGlobalIsFinite * Enable recommended rule suspicious/noGlobalIsNan * Enable recommended rule suspicious/noPrototypeBuiltins * Enable recommended rule suspicious/noShadowRestrictedNames * Enable recommended rule suspicious/useDefaultSwitchClauseLast * Enable recommended rule suspicious/useGetterReturn * Enable recommended rule style/noUnusedTemplateLiteral * Convert default case to unreachable code * Enable recommended rule complexity/noForEach * Enable recommended rule complexity/noThisInStatic * Enable recommended rule complexity/useFlatMap * Enable recommended rule complexity/useOptionalChain * Enable recommended rule complexity/useRegexLiterals * Reorganize the config structure * Fix few typos * Revert "Enable recomended rule performance/noDelete" This reverts commit 6cc060b. * Fix formatting * Enable recommended rule style/noUselessElse * Enable recommended rule complexity/useLiteralKeys * Enable recommended rule complexity/useArrowFunction * Fix few types * Fix a test file * Fix formatting * Enable recommended rule suspicious/noImplicitAnyLet * Enable recommended rule complexity/noUselessEmptyExport * Fix types * Fix unti tests * Fix unit test * Fix lint error * Fix a unit test pattern * Fix formatting
|
🎉 This PR is included in v1.23.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Make sure all recommended rules from biomejs are enabled.
Description