Added the eslint rule to help with import order and ran the fix - #3885
Merged
Conversation
dadepo
requested review from
dapplion,
g11tech,
twoeths and
wemeetagain
as code owners
April 5, 2022 15:08
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Apr 5, 2022
| import * as block from "./block"; | ||
| import * as pool from "./pool"; | ||
| import * as state from "./state"; | ||
| import {RoutesData, ReturnTypes, reqEmpty, ContainerData} from "../../utils"; |
Contributor
There was a problem hiding this comment.
So we actually want the closest modules at the bottom, so the correct order would be the original one. @dadepo can you try to tweak the rule to achieve that?
Codecov Report
@@ Coverage Diff @@
## master #3885 +/- ##
===========================================
+ Coverage 0 36.60% +36.60%
===========================================
Files 0 325 +325
Lines 0 8975 +8975
Branches 0 1407 +1407
===========================================
+ Hits 0 3285 +3285
- Misses 0 5547 +5547
- Partials 0 143 +143 |
twoeths
previously approved these changes
Apr 7, 2022
Member
|
Can we block this until we land ESM support? This will be a bit annoying for that PR by creating a bunch of confiicts. |
wemeetagain
added
the
status-blocked
wemeetagain
removed
the
status-blocked
Member
|
ready for rebase off master |
…conflict. Still new to review if the eslint works as before
wemeetagain
previously approved these changes
May 25, 2022
Member
|
tests still erroring |
Member
|
I think I made conflicts with #4064 |
wemeetagain
previously approved these changes
May 26, 2022
dapplion
previously requested changes
May 26, 2022
| get attnets(): BitArray { | ||
| return this._metadata.attnets; | ||
| } | ||
| set attnets(attnets: BitArray) { |
Contributor
There was a problem hiding this comment.
Please preserve whitespace in all changes of this file. After each method and at least 1 new line
| return this.state === SyncState.Synced; | ||
| } | ||
|
|
||
| get state(): SyncState { |
Contributor
There was a problem hiding this comment.
Why all this changes in a PR that's clearly scoped to "import order"? Could you please revert this commit and do this changes if applicable? We have really never followed the member-ordering rule in Lodestar
wemeetagain
approved these changes
May 27, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
In a previous PR it was pointed out that the preferred order for imports should be:
This PR enforces this via eslint to make it more applicable. It also updates existing import to adhere with the import rule