-
Notifications
You must be signed in to change notification settings - Fork 373
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
Replace TSLint with ESLint and Prettier #2112
Replace TSLint with ESLint and Prettier #2112
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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 tried the configured linter in the various packages.
- I got a lot of
Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
warnings reported. Do you intend to fix them with this PR or shall we disable the rule for now to not make the warnings meaningless? This could be handled in a follow up PR then - There are a lot of
Do not access Object.prototype method 'hasOwnProperty' from target object no-prototype-builtins
errors. Do you intend to fix them with this PR? - There are warning like
'_key' is defined but never used @typescript-eslint/no-unused-vars
. Personally I prefer to specify an exception to this rule for variables starting with an underscore. Do you agree? If yes, can you add it? - The
lint
script is also linting config files likerollup.config.js
. This should either be avoided or they need specific rules. Personally I would not lint these files. - There is warning when running
lint
in the react packagesWarning: React version not specified in eslint-plugin-react settings. See https://github.com/jsx-eslint/eslint-plugin-react#configuration
- Some of the test files should probably be excluded as they do things like
require
which the linter doesn't like
Also:
- Can you add markdownlint to the recommended extensions?
- There is no linter yet in the vue packages
I think the PR should consist of three commits in the end:
- A commit adding all the configurations
- A commit applying all autofixable resolutions
- A commit fixing all remaining errors and warnings
705a1c2
to
6ac0f06
Compare
@sdirix I improved the config. See the commit messages for the changes. I am going to squash the changes to have the three suggested commits in the end |
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 so far
- Remove TSLint dependencies and config - Add ESLint and Prettier dependencies and config for all packages - Add markdownlint to recommended extensions - Use ESLint 7.x instead of 8.x because the Angular eslint plugin for Angular 12 does not work with ESLint 8.x Disabled rules for to be re-enabled later: - explicit any warnings - no-prototype-builtins rule - import/no-named-as-default for 2 packages - vanilla-renderers - material-renderers Part of eclipsesource#1551
6ac0f06
to
ee119cb
Compare
ee119cb
to
32b8ec2
Compare
Fix remaining ESLint issues with these exceptions that will be handled in a follow up - import/no-named-as-default in vanilla-renderers, material-renderers - no-prototype-builtins - @typescript-eslint/no-explicit-any - typescript-eslint/ban-types Part of eclipsesource#1551
32b8ec2
to
d447946
Compare
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! Thanks!
#1551