Skip to content
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

Migrate tslint to eslint #1590

Merged
merged 21 commits into from
May 3, 2020
Merged

Migrate tslint to eslint #1590

merged 21 commits into from
May 3, 2020

Conversation

tanettrimas
Copy link
Contributor

Summary

Fixes #1566

Making changes to configuration files where we want to migrate from deprecated tslint to eslint.

Does this PR introduce a breaking change?

  • Yes
  • No

Currently, there are some errors in the linting which breaks tests, but those are regarding prettier formatting mostly. Other errors have been put to warning. These can either be handled or put to off in .eslintrc

Other information

I was initially considering to add eslint-plugin-import to lint some errors regarding import syntax, ordering and stuff but that heavily degraded the performance of the linting.

Here is with:

Skjermbilde 2020-05-01 kl  18 07 16

Without:

Skjermbilde 2020-05-01 kl  18 22 25

tanettrimas and others added 9 commits May 1, 2020 22:02
This change is detect where there might be issues with referencing ´this´

from the rule definition (https://github.com/typescript-eslint/typescript-eslint/blob/v2.30.0/packages/eslint-plugin/docs/rules/unbound-method.md) :

`Warns when a method is used outside of a method call.

Class functions don't preserve the class scope when passed as standalone variables.`
Bumps [jest](https://github.com/facebook/jest) from 25.5.3 to 25.5.4.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](jestjs/jest@v25.5.3...v25.5.4)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
@ahnpnl
Copy link
Collaborator

ahnpnl commented May 2, 2020

look quite a lot files to review but it's easier to review against next branch 👍

@tanettrimas
Copy link
Contributor Author

tanettrimas commented May 2, 2020

Sorry about that, bear in mind that the changes to existing files are minimal and should only be linting fixes with tabs and such :) The most noticable changes are done in

  • Package.json
  • .eslintrc
  • .eslintrcignore
  • tsconfig.json

@coveralls
Copy link

coveralls commented May 2, 2020

Pull Request Test Coverage Report for Build 4670

  • 39 of 40 (97.5%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 92.865%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/util/backports.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 4642: -0.004%
Covered Lines: 1067
Relevant Lines: 1103

💛 - Coveralls

Copy link
Collaborator

@ahnpnl ahnpnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks strange to me that after migration the line formatting seems to change for all js files. Was it caused by the new rules which are merged from tslint rules ?

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/transformers/hoist-jest.ts Outdated Show resolved Hide resolved
e2e/__helpers__/test-case/types.ts Outdated Show resolved Hide resolved
e2e/__helpers__/test-case/utils.ts Outdated Show resolved Hide resolved
e2e/__helpers__/test-case/utils.ts Outdated Show resolved Hide resolved
src/index.spec.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@tanettrimas
Copy link
Contributor Author

@ahnpnl I think all the review comments are done in the last commit :)

Copy link
Collaborator

@ahnpnl ahnpnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are still a few comments :)

tsconfig.json Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@ahnpnl
Copy link
Collaborator

ahnpnl commented May 2, 2020

I'm reading warning logs in CI. Probably we need to fix them, either disable eslint check for code or fix the code itself.

.eslintrc.js Outdated Show resolved Hide resolved
@tanettrimas
Copy link
Contributor Author

it looks strange to me that after migration the line formatting seems to change for all js files. Was it caused by the new rules which are merged from tslint rules ?

It was my vscode that messed things up, I had wrong tabs :)

.eslintrc.js Outdated Show resolved Hide resolved
@ahnpnl
Copy link
Collaborator

ahnpnl commented May 2, 2020

@kulshekhar would you please take a look if any rules we need to adjust ?

.eslintignore Outdated Show resolved Hide resolved
Copy link
Owner

@kulshekhar kulshekhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from one minor issue, this looks good!

@ahnpnl ahnpnl merged commit d53134d into kulshekhar:next May 3, 2020
@ahnpnl ahnpnl mentioned this pull request May 3, 2020
@tanettrimas tanettrimas deleted the migrate-tslint-to-eslint branch May 3, 2020 16:59
ahnpnl pushed a commit that referenced this pull request May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants