Upgrade TSLint to ESLint, rename package tslint -> lint #26
Conversation
834de06 to
5756483
Compare
|
Hi Ryan,
If you agree: I think it would be worth to do the switch, maybe always along some breaking releases. So do we want to start here on this occasion with the |
|
Yeah sounds great, I think it’s a good idea. I think without the “ing” is fine so “-format” and “-lint”, if you agree. Should we review and merge this then I’ll do a separate PR for the rest? |
|
Also, for this PR, should I port over the existing rules on top of the recommended base set? I’m not sure the history of choosing the existing rules. |
|
We can very well leave the "ing". 🙂 Review and merge this first also sounds fine. Not sure on the rule set - this is the first time I get in touch with this TSLint -> ESLint deprecation - how much do rules differ between the two? |
I migrated the existing rules from This is the current {
"extends": "typestrict",
"rules": {
"no-console": {
"severity": "warning"
},
"no-debugger": true,
"mocha-avoid-only": true,
"prefer-const": true,
"no-var-keyword": true,
"interface-name": [true, "never-prefix"],
"no-commented-code": {
"severity": "warning"
},
"no-use-before-declare": false
}
}The biggest thing we're missing in eslint is I was able to convert all of the existing rules except This is the resulting module.exports = {
parser: "@typescript-eslint/parser",
plugins: ["@typescript-eslint"],
env: {
es6: true,
node: true
},
ignorePatterns: ["node_modules/**/*", "**/node_modules/**/*", "dist/**/*"],
extends: [
"eslint:recommended",
"plugin:@typescript-eslint/eslint-recommended",
"plugin:@typescript-eslint/recommended",
"plugin:@typescript-eslint/recommended-requiring-type-checking",
"plugin:prettier/recommended"
],
rules: {
"no-console": "warn",
"no-debugger": "error",
"prefer-const": "error",
"no-var": "error",
"@typescript-eslint/no-use-before-define": "error",
"@typescript-eslint/interface-name-prefix": [
"error",
{
prefixWithI: "never"
}
]
},
parserOptions: {
sourceType: "module"
}
}; |
|
Ok, great!
Generally: this was our very first try to have some more explicit unification and agreement on the linting rules. Don't have anything specific in mind and this generally worked well. We can nevertheless very much do some between conclusion here, improve on stuff or remove things which didn't work so well. Feel very free to add your own recommendations to the discussion. We were a limited group of people discussing this and there is very much room for change and additional voices, eventually @evertonfraga and/or @cgewecke also want to have a word here. I wouldn't want to merge here before EthCC I think, we can let this sink in a bit further. This is also a good topic to discuss and exchange about at EthCC face-to-face. Feel free to do some continued work here though if necessary. |
|
FYI: TypeStrict just migrated to ESLint! krzkaczor/TypeStrict#19 |
|
Amazing, thanks @krzkaczor! I will update this PR to include it 😎 |
|
@ryanio just give an explicit comment once this is open again for review, might be overlooked otherwise |
f61dc1e to
5c4f581
Compare
|
@holgerd77 @krzkaczor ok, just pushed some updates and this is ready for another review! I tried to do a test run in |
|
What is the status here? |
de09fe6 to
af26ac5
Compare
|
@ryanio Thanks for the update! 😄 Is this now ready for review? |
|
(overall status is a bit confusing since the current change request is still active and displayed) |
|
Hm thanks yes @evertonfraga can you update your review if the updated code looks good? I am going to try to use |
|
Sorry, I totally missed this notification. Will resume with testing tomorrow! |
|
ok I made some updates and I got This is what I needed to do:
module.exports = {
parserOptions: {
project: './tsconfig.json',
}
};and run I got 97 lint problems however they seem to be just The only last thing I would like to figure out if there is a way to not need |
|
Right, thanks for the work here @ryanio! It took me a little while get the whole picture of this package and its current state. I am testing with VM, made some changes in These are the current imposed requirements to make the
I made some basic error handling for (1). But I have a feeling that we must do something about the other bullet points. Suggestions:
"scripts": {
"codecheck": "ethereumjs-config-lint && ethereumjs-config-format"
} |
|
@evertonfraga looks great, thanks for the rebase! |
|
Not really able to catch up with all the developments here, but from first sight things seem to be going in a good direction, so just another 👍 to go ahead here once you are confident on the changes and merge. |
|
Thanks! I'd like to make a test run on all tasks today. This series of PRs have touched every internal package. |
There was a problem hiding this comment.
ESLint didn't work for me.
The extends: @ethereumjs/config-lint clause in the consumer package gets transformed to @ethereumjs/eslint-config-config-lint.
According to ESLint docs:
The module name can also be customized, just note that when using scoped modules it is not possible to omit the eslint-config- prefix.
https://eslint.org/docs/developer-guide/shareable-configs#npm-scoped-modules
🤷♂️
Edit: unless an alternative to keep the same name is possible, we should rename @ethereumjs/config-lint to @ethereumjs/eslint-config-<something>
|
By the way, I made this little script, to help migrate the package changes. There's another one to help link the packages using As anything I have to do in the monorepo is x6, this kind of thing is super handy to me. |
|
@ryanio I followed your instructions from here, and I got the same result as you:
But the 97 errors described indicate that ESLint is not configured properly, as it presents parsing errors, as it does not extend the one defined in
|
|
Additionally, I found some eslint plugins installation mismatch. Can you provide updated information on how to setup this plugin? |
|
@evertonfraga thanks for your patience with this PR 😄 This is currently working for me with latest updates to branch, if you want to give it a try: In In Run Create module.exports = {
extends: '@ethereumjs/eslint-config-helper',
parserOptions: {
project: './tsconfig.eslint.json',
},
}Create {
"extends": "./tsconfig.json",
"include": ["src/**/*.ts", "test/**/*.ts", ".eslintrc.js", "prettier.config.js", "typedoc.js"]
}(See typescript-eslint/typescript-eslint#1723 (comment) for more on why In account, run ( My results: |
|
Sure, I can rename it now. I am testing it and doing a test integration to the VM. |
re-add deps needed add `--resolve-plugins-relative-to` to sh script
8c141aa to
24f1e28
Compare
|
@evertonfraga just so that there won't stay any confusion: did you leave this for @ryanio to merge or is there anything explicitly blocking this or other reasoning to leave this open for now? |
|
I am making integration tests with the monorepo and will merge this one soon |
TSLint is now deprecated in favor of ESLint.
This PR is a breaking change, and does the following:
ethereumjs-config-tslinttoethereumjs-config-linttypestrict,eslint,typescript-eslintandprettierHow repos using this package should update
Once published in npm, update in
package.json:Add
.eslintrc.jswith:Use bin cmd:
ethereumjs-config-lintThis PR also aims to close #19 and ethereumjs/ethereumjs-monorepo#640