-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Update ESLint shared config to v3 #9274
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
Conversation
40f3a5f to
9e54a7c
Compare
| @@ -1,4 +1,9 @@ | |||
| import ObservableStore from 'obs-store' | |||
|
|
|||
| /* eslint-disable import/first,import/order */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these rules disabled here because they conflicted with each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're disabled here because they both apply to the following code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import/firstwants therequireandimports to moveimport/orderwants therequirecall alone to move
They're both disabled here and re-enabled as soon as ESLint will allow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The e2e tests break if I move the require call so I don't want to move it
| ], | ||
| ) | ||
|
|
||
| return t('connectTo', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What motivated this change 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, habit—I'd meant to only remove the extra newline but got carried away
| const _fetch = timeout | ||
| ? fetchWithTimeout({ timeout }) | ||
| : window.fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are extraneous, not suggested by ESLint
| version, | ||
|
|
||
| migrate: function (originalVersionedData) { | ||
| migrate (originalVersionedData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What syntax is this O_O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with ECMAScript 2015, a shorter syntax for method definitions on objects initializers is introduced. It is a shorthand for a function assigned to the method's name.
| manifestTasks.dev, | ||
| reload, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unexpected. I prefer the older format, and I'm not sure what could have caused this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first arg to the function is mis-aligned, causing the auto-fix to do this. I'll fix this with db7f5f2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same problem is present in many other places, but I'm not gonna block on it. Hopefully prettier will fix it.
test/lib/createTxMeta.js
Outdated
| status: 'unapproved', | ||
| txParams: {}, | ||
| }, partialMeta) | ||
| txParams: {}, ...partialMeta, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird to see multi-line object literals with things doubled up on the same line like this 🤔 I would have expected this to be disallowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is allowed. I've fixed it with 4ae9694.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same problem is present in many other places, but I'm not gonna block on it. Hopefully prettier will fix it.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* origin/develop: (137 commits) Use @metamask/eslint-config@3.1.0 (#9275) Standardize scss import practices (#9183) Update ESLint shared config to v3 (#9274) Add lock icon to default networks (#9269) Adds toPrecisionWithoutTrailingZeros utility (#9270) Hide gas estimate on non-main network (#9189) Move the mascot component to its own directory (#9272) Use @metamask/controllers@2.0.5 (#9266) Fix padding, alignment of actionable-message; add left aligned story Code cleanup and simplification for actionable-message component Adds actionable message component and stories Fix lint issues (#9265) Fix prefer-destructuring issues (#9263) colocate confirm-decrypt-message page styles (#9252) Tidy up Migrator tests (#9264) Adds pulse loader component (#9259) Fix import/order issues (#9239) Fix radix issues (#9247) New info tooltip component (#9180) Improve scss naming ...
#30661) ## **Description** @davidmurdoch requested this feature here: #30440 (review) Also adds to the VSCode GitLens settings. If a Cursor user could help with the Cursor settings, that would be much appreciated. We should discuss: - The inclusion of #17092, as I'm undecided about it - Whether it's appropriate to automatically execute `git config blame.ignoreRevsFile .git-blame-ignore-revs` in `postinstall`. It writes to the local `.git/config` file in your `metamask-extension` folder, so it's only changing that one folder. Command to get commits with over 200 file changes ``` git log --pretty=format:"%H %s" --shortstat | awk '{if ($1 ~ /^[0-9]+$/) {num = $1 + 0; if (num > 200 && current_hash !~ /Revert/) print num " " current_hash} else current_hash = $0}' | sort -nr ``` [](https://codespaces.new/MetaMask/metamask-extension/pull/30661?quickstart=1) ## **Related issues** David Murdoch request: #30440 (review) ## **Ignored PRs** - #6304 - #7730 - #8023 - #8056 - #8595 - #9239 - #9274 - #10358 - #10655 - #10911 - #17092 - #22639 - #22531 - #30440 <!--## **Manual testing steps** ## **Screenshots/Recordings** ## **Pre-merge author checklist** ## **Pre-merge reviewer checklist**--> --------- Co-authored-by: David Murdoch <187813+davidmurdoch@users.noreply.github.com>
Closes #8982
This PR updates the ESLint config to the latest version, v3.
There are two commits with fixes, one that's automated and one that was manual. Only the first and last commit require review.
I disabled the following:
default-param-lastwas too annoying to fixrequire-atomic-updateswas also too annoying to fiximport/no-unassigned-importall the instances of this in the codebase are legitimate—I don't think we should enable it if every time it flags something we need to disable itI'm open to enabling those in separate PRs in the future, should someone feel inclined.