-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Move samples and template from TSLint to ESLint #561
Move samples and template from TSLint to ESLint #561
Conversation
Is this .eslint.js set of rules okish? This is 100% tweakable, I just did what felt right, but maybe we could use a more strict set of rules. |
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.
Sorry it took so long. I'm a little overwhelmed these weeks.
Thank you for sharing your changes in different commits by the way. It really makes the review easier. 👍
I recently changed Foal CI from Travis to Github Actions, this is why no tests are running in the web hooks. We have to run them manually on local.
Review steps
- Review each commit. ✅
- Clone locally the fork. ✅
- Check that
npm run lint
succeeds in each sample. ✅ - Run all unit and acceptance tests. ✅
- Search for all mentions of
tslint
,eslint
andparseInt
in the repository. ✅ - Check that all the new dependencies are required. ✅
- Read the documentation of the dependencies. ✅
- Generate a project with the CLI. 🔴
When building @foal/cli
, the command copyfiles
does not copy by default the templates starting with a dot. We have to specify it manually by adding the flag -a
in packages/cli/package.json
-> copyfiles -a -u 3
.
- In the new project, test that the commands
npm run lint
andnpm run lint:fix
work. ✅ - Check that VSCode extension works as the documentation suggests. 🔴
We need to tell users to extend their settings here to make it work (to support both TypeScript and the auto-fix feature):
"eslint.validate": [
{ "language": "typescript", "autoFix": true }
],
"eslint.autoFixOnSave": true
- Check that migration files are ignored. 🔴
Migrations are generated with different coding rules. We should ignore them by default not to get lint errors when getting started with Foal. Developers will still be able to change this if they want to after.
To do so, we can add this in the package.json
templates: "eslintIgnore": [ "src/migrations/*.ts" ]
- Run
e2e_test.sh
locally. 🔴
Generated entities do not pass the linting.
Do not use "// @ts-ignore" comments because they suppress compilation errors.
'Column' is defined but never used
We need to replace all // @ts-ignore : 'Column' is declared but its value is never read.
with // eslint-disable-next-line @typescript-eslint/no-unused-vars
.
- Check that the files eslintrc.js and tslint.json are equivalent. ✔️
I took a look at the rules that you migrated from TSLint as well as the others that you added to « fix » the recommended ones from ESLint. All seems good. Since the tests are passing, I think we can reasonably consider that both files are equivalent.
Good work! There are some stuff to change/add and we’re good! 👍
I think I will wait until eslint/eslint#12274 is merged so don't need one more |
Ok, it has landed! ESLint 6.7.0 has been released :D |
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 👍
Great idea to use the .eslintrc
file to ignore migrations!
Issue
TSLint is being deprecated (palantir/tslint#4534), and this PR fixes #555.
Solution and steps
Checklist