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

[#IP-86] tslint to eslint migration #14

Merged
merged 24 commits into from Apr 15, 2021
Merged

Conversation

michaeldisaro
Copy link
Contributor

@michaeldisaro michaeldisaro commented Mar 26, 2021

List of Changes

  • Deleted tslint.json file
  • Removed tslint packages from package.json
  • Created .eslintignore ignore file
  • Created .eslintrc.js configuration
  • Added some lint exclusions to .eslintrc.js to avoid a deep refactoring of working code
  • Changed tslint's "rule disabling" comments with eslint closest ones.

Motivation and Context

We want to migrate to eslint because tslint is deprecated.

How Has This Been Tested?

It has been tested by performing:

  • yarn install --frozen-lockfile
  • yarn build
  • yarn lint
  • yarn test

Types of changes

  • Chore (nothing changes by a user perspective)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Contributor

@balanza balanza left a comment

Choose a reason for hiding this comment

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

Just minor changes

.devops/code-review-pipelines.yml Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
michaeldisaro and others added 4 commits March 29, 2021 09:33
Co-authored-by: Emanuele De Cupis <emanuele.decupis@pagopa.it>
Co-authored-by: Emanuele De Cupis <emanuele.decupis@pagopa.it>
@pagopa-github-bot
Copy link
Contributor

pagopa-github-bot commented Apr 6, 2021

Warnings
⚠️ This PR changes a total of 1736 LOCs, that is more than a reasonable size of 250. Consider splitting the pull request into smaller ones.
⚠️ Please include a Pivotal story at the beginning of the PR title (see below).

eslint-plugin-prettier

Author: Teddy Katz

Description: Runs prettier as an eslint rule

Homepage: https://github.com/prettier/eslint-plugin-prettier#readme

Createdabout 4 years ago
Last Updatedabout 7 hours ago
LicenseMIT
Maintainers4
Releases26
Direct Dependenciesprettier-linter-helpers
Keywordseslint, eslintplugin, eslint-plugin and prettier
README

eslint-plugin-prettier Build Status

Runs Prettier as an ESLint rule and reports differences as individual ESLint issues.

If your desired formatting does not match Prettier’s output, you should use a different tool such as prettier-eslint instead.

Please read Integrating with linters before installing.

Sample

error: Insert `,` (prettier/prettier) at pkg/commons-atom/ActiveEditorRegistry.js:22:25:
  20 | import {
  21 |   observeActiveEditorsDebounced,
> 22 |   editorChangesDebounced
     |                         ^
  23 | } from './debounced';;
  24 |
  25 | import {observableFromSubscribeFunction} from '../commons-node/event';


error: Delete `;` (prettier/prettier) at pkg/commons-atom/ActiveEditorRegistry.js:23:21:
  21 |   observeActiveEditorsDebounced,
  22 |   editorChangesDebounced
> 23 | } from './debounced';;
     |                     ^
  24 |
  25 | import {observableFromSubscribeFunction} from '../commons-node/event';
  26 | import {cacheWhileSubscribed} from '../commons-node/observable';


2 errors found.

./node_modules/.bin/eslint --format codeframe pkg/commons-atom/ActiveEditorRegistry.js (code from nuclide).

Installation

npm install --save-dev eslint-plugin-prettier
npm install --save-dev --save-exact prettier

eslint-plugin-prettier does not install Prettier or ESLint for you. You must install these yourself.

Then, in your .eslintrc.json:

{
  "plugins": ["prettier"],
  "rules": {
    "prettier/prettier": "error"
  }
}

Recommended Configuration

This plugin works best if you disable all other ESLint rules relating to code formatting, and only enable rules that detect potential bugs. (If another active ESLint rule disagrees with prettier about how code should be formatted, it will be impossible to avoid lint errors.) You can use eslint-config-prettier to disable all formatting-related ESLint rules.

This plugin ships with a plugin:prettier/recommended config that sets up both the plugin and eslint-config-prettier in one go.

  1. In addition to the above installation instructions, install eslint-config-prettier:

    npm install --save-dev eslint-config-prettier
  2. Then you need to add plugin:prettier/recommended as the last extension in your .eslintrc.json:

    {
      "extends": ["plugin:prettier/recommended"]
    }

    You can then set Prettier's own options inside a .prettierrc file.

Exactly what does plugin:prettier/recommended do? Well, this is what it expands to:

{
  "extends": ["prettier"],
  "plugins": ["prettier"],
  "rules": {
    "prettier/prettier": "error",
    "arrow-body-style": "off",
    "prefer-arrow-callback": "off"
  }
}
  • "extends": ["prettier"] enables the config from eslint-config-prettier, which turns off some ESLint rules that conflict with Prettier.
  • "plugins": ["prettier"] registers this plugin.
  • "prettier/prettier": "error" turns on the rule provided by this plugin, which runs Prettier from within ESLint.
  • "arrow-body-style": "off" and "prefer-arrow-callback": "off" turns off two ESLint core rules that unfortunately are problematic with this plugin – see the next section.

arrow-body-style and prefer-arrow-callback issue

If you use arrow-body-style or prefer-arrow-callback together with the prettier/prettier rule from this plugin, you can in some cases end up with invalid code due to a bug in ESLint’s autofix – see issue #65.

For this reason, it’s recommended to turn off these rules. The plugin:prettier/recommended config does that for you.

You can still use these rules together with this plugin if you want, because the bug does not occur all the time. But if you do, you need to keep in mind that you might end up with invalid code, where you manually have to insert a missing closing parenthesis to get going again.

If you’re fixing large of amounts of previously unformatted code, consider temporarily disabling the prettier/prettier rule and running eslint --fix and prettier --write separately.

Options

Note: While it is possible to pass options to Prettier via your ESLint configuration file, it is not recommended because editor extensions such as prettier-atom and prettier-vscode will read .prettierrc, but won't read settings from ESLint, which can lead to an inconsistent experience.

  • The first option:

    • An object representing options that will be passed into prettier. Example:

      "prettier/prettier": ["error", {"singleQuote": true, "parser": "flow"}]

      NB: This option will merge and override any config set with .prettierrc files

  • The second option:

    • An object with the following options

      • usePrettierrc: Enables loading of the Prettier configuration file, (default: true). May be useful if you are using multiple tools that conflict with each other, or do not wish to mix your ESLint settings with your Prettier configuration.

        "prettier/prettier": ["error", {}, {
          "usePrettierrc": false
        }]
      • fileInfoOptions: Options that are passed to prettier.getFileInfo to decide whether a file needs to be formatted. Can be used for example to opt-out from ignoring files located in node_modules directories.

        "prettier/prettier": ["error", {}, {
          "fileInfoOptions": {
            "withNodeModules": true
          }
        }]
  • The rule is autofixable -- if you run eslint with the --fix flag, your code will be formatted according to prettier style.


Contributing

See CONTRIBUTING.md

@pagopa/eslint-config

Author: Unknown

Description: This package provide the following ESLint custom rules for Typescript projects.

Homepage: https://github.com/pagopa/eslint-rules#readme

Created3 months ago
Last Updatedabout 16 hours ago
LicenseMIT
Maintainers5
Releases6
Direct Dependencies@typescript-eslint/eslint-plugin, @typescript-eslint/eslint-plugin-tslint, @typescript-eslint/parser, eslint, eslint-config-prettier, eslint-plugin-extra-rules, eslint-plugin-fp, eslint-plugin-functional, eslint-plugin-import, eslint-plugin-jsdoc, eslint-plugin-no-credentials, eslint-plugin-prefer-arrow, eslint-plugin-prettier, eslint-plugin-react, eslint-plugin-sonarjs and prettier
README

PagoPA ESLint config

This package provide the following ESLint custom rules for Typescript projects.

  • recommendend
  • react
  • strong

This repository replace italia-tslint-rules after TSLint deprecation.

The following TSLint rules (included inside italia-tslint-rules) are not supported for eslint at the moment and are missing in this package:

bool-param-default
max-union-size
no-accessor-field-mismatch
no-array-delete // Mitigated by no-delete
no-case-with-or
no-dead-store
no-duplicate-in-composite
no-empty-array
no-extra-semicolon // Mitigated by prettier
no-empty-destructuring // Mititgated by no-empty-pattern
no-gratuitous-expressions
no-hardcoded-credentials
no-ignored-initial-value // Mitigated by no-param-reassign, no-let
no-in-misuse
no-inferred-empty-object-type
no-invalid-await // Mititgated by await-thenable
no-invariant-return
no-misleading-array-reverse // Mitigated by immutable-data
no-misspelled-operator // Mitigated by prettier
no-nested-switch
no-nested-template-literals
no-statements-same-line // Mitigated by prettier
no-try-promise
no-tslint-disable-all
no-unconditional-jump
no-undefined-argument
no-unenclosed-multiline-block // Mitigated by prettier
no-unthrown-error
no-unused-array // Mitigated by no-unused-vars
no-useless-increment
no-useless-intersection
prefer-promise-shorthand
promise-must-complete
use-primitive-type

This list has been produced following these steps:

  1. Follow TSLint to ESLint migration guide on the project io-backend
  2. Running npx tslint-to-eslint-config that produces a list of TSLint rules not available for ESLint (77 rules at the moment)
  3. Manually check each one for an alternative ESLint rules/plugins
  4. Verify each alternative ESLint rule/plugin on a testing file

Usage

Installation and Configuration

To use this package install as devDependecy inside any typescript project with

yarn install -D @pagopa/eslint-config

Create on the project an .eslintrc.js file with the following content

module.exports = {
  "extends": [
    "@pagopa/eslint-config/strong",
  ],
  "rules": {
    // Any project level custom rule
  }
}

Add inside the package.json file a lint and optionally a lint-autofix script as:

"scripts": {
  "lint": "eslint . -c .eslintrc.js --ext .ts,.tsx",
  "lint-autofix": "eslint . -c .eslintrc.js --ext .ts,.tsx --fix",
  ...
}

Migration from TSLint

Remove from the package.json every tslint reference:

  • tslint
  • italia-tslint-rules

Replace all the // tslint:disable-next-line with the proper // eslint-disable-next-line comment. If are present some // tslint:disable replace it with /* eslint-disable */ at the top of the file.

If you need to disable ESLint for some files create .eslintignore file with the list of folders or files that must be excluded from lint process. Copy the exclusion from tslint.json linterOptions.exclude

Delete all tslint related files (es. tslint.json).

Run yarn lint-autofix to refactorize the code automatically with all the auto-fixable ESLint rules.

New dependencies added: @pagopa/eslint-config and eslint-plugin-prettier.

Example of PR titles that include pivotal stories:

  • single story: [#123456] my PR title
  • multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS against 02689b3

@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #14 (02689b3) into master (b2819af) will decrease coverage by 0.47%.
The diff coverage is 87.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
- Coverage   87.97%   87.50%   -0.48%     
==========================================
  Files          22       22              
  Lines         424      416       -8     
  Branches       42       43       +1     
==========================================
- Hits          373      364       -9     
- Misses         50       51       +1     
  Partials        1        1              
Impacted Files Coverage Δ
...HCreateOrUpdateInstallationCallActivity/handler.ts 100.00% <ø> (ø)
HandleNHDeleteInstallationCallActivity/handler.ts 100.00% <ø> (ø)
HandleNHNotificationCallOrchestrator/handler.ts 95.23% <ø> (ø)
HandleNHNotifyMessageCallActivity/handler.ts 100.00% <ø> (ø)
utils/durable/orchestrators/index.ts 83.78% <ø> (ø)
utils/durable/orchestrators/log.ts 77.77% <ø> (ø)
utils/durable/orchestrators/returnTypes.ts 100.00% <ø> (ø)
utils/durable/utils.ts 100.00% <ø> (ø)
utils/healthcheck.ts 42.42% <ø> (ø)
utils/durable/activities/returnTypes.ts 69.56% <50.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cf783c...02689b3. Read the comment docs.

@michaeldisaro
Copy link
Contributor Author

@balanza I have fixed yarn.lock and merged master resolving conflicts. Check have passed.

HandleNHNotificationCallActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
utils/activity.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@michaeldisaro michaeldisaro left a comment

Choose a reason for hiding this comment

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

I made requested changes

Copy link
Contributor

@balanza balanza left a comment

Choose a reason for hiding this comment

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

It seems ok to me. Can you please resolve conflicts, so we can merge?

Copy link
Contributor Author

@michaeldisaro michaeldisaro left a comment

Choose a reason for hiding this comment

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

It should be all ok. @gquadrati can you check that everything is compliant with your new changes?

Copy link
Contributor

@gquadrati gquadrati left a comment

Choose a reason for hiding this comment

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

LGTM apart of two minor changes

HandleNHNotificationCall/handler.ts Outdated Show resolved Hide resolved
HandleNHNotifyMessageCallActivity/handler.ts Outdated Show resolved Hide resolved
michaeldisaro and others added 5 commits April 14, 2021 09:43
Co-authored-by: gquadrati <75862507+gquadrati@users.noreply.github.com>
Co-authored-by: gquadrati <75862507+gquadrati@users.noreply.github.com>
@balanza balanza self-requested a review April 15, 2021 08:14
@balanza balanza merged commit 45a490d into master Apr 15, 2021
@balanza balanza deleted the io-core-ip-86-tslint2eslint branch April 15, 2021 08:14
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.

None yet

4 participants