-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
[code-infra] Closer sync with eslint config of codebase #3441
Conversation
rules: { | ||
'import/no-cycle': ['error', { ignoreExternal: true }], |
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.
Removed seems to duplicate
rules: { | ||
'import/no-cycle': ['error', { ignoreExternal: true }], | ||
'no-restricted-imports': ['error', noRestrictedImports], |
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.
Added until fixed in Toolpad
.eslintrc.js
Outdated
// Personal preference | ||
'no-underscore-dangle': 'off', |
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.
Seems marginally used
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.
There is a reason. examples don't contain eslint. So the code shouldn't contain eslint comments.
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 makes sense. Should we move this rule to the share eslint config then? It feels like we want this for all /examples folders through MUI codebase.
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 good with that as well
'import/prefer-default-export': ['off'], | ||
// TODO move to @mui/monorepo, codebase is moving away from default exports | ||
'import/prefer-default-export': 'off', |
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.
Syntax convention (['off'] -> 'off')
}, | ||
], | ||
'@typescript-eslint/return-await': 'off', | ||
'no-restricted-imports': ['error', noRestrictedImports], |
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.
moved to a variable, noRestrictedImports
, to be able to extend it in specific paths.
}, | ||
overrides: [ | ||
...baseline.overrides, |
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 main difference
@@ -1,3 +1,4 @@ | |||
/* eslint-disable no-underscore-dangle */ |
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.
In general we don't add eslint disable comments in examples, but turn off the rule in an eslint override.
Reason is that eslint is not used/installed in the example, so it shouldn't contain eslint comments after I download it.
Did you see that eslint is in the process of deprecating its config format? The new format seems considerably easier to understand in terms of extending lint configs. |
I briefly looked at it, but saw we are on ESLint v8 not v9 so I ignored it. I have pushed in the past to not add any of those: https://github.com/mui/material-ui/blob/next/packages/api-docs-builder/.eslintrc.js. I'm happy it won't be possible anymore. If we want to prepare the migration, we could remove more of those. |
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.
Fixed the naming and brought back the rule for examples. With clearer comment. Want to merge this as I see PRs coming in that would fail the new naming rules.
A step to simplify the eslint config in the codebase.