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

Support ESLint 8.x #302

Merged
merged 15 commits into from Nov 30, 2021
Merged

Support ESLint 8.x #302

merged 15 commits into from Nov 30, 2021

Conversation

yassin-kammoun-sonarsource
Copy link
Contributor

Fixes #286

gboer and others added 7 commits November 22, 2021 15:36
Update packages to support eslint 8 and fix unsupported code error that results in 'Module specifier must be a string literal' parse error.
Update @typescript-eslint/experimental-utils, to be in line with eslint 8.x types.
Update typescript to 4.4.4, after updating of @typescript-eslint/experimental-utils, because it kept failing due to errors in 'node_modules/@typescript-eslint/types/dist/ast-spec.d.ts' and node_modules/typescript/lib/typescript.d.ts.
Remove typings/eslint.d.ts in favor of @types/eslint.
Remove all categories, since that's removed in eslint 8.
Fix spacing after/before imports, to fix failing eslint rule.
Replace 'message' with 'messageId', since 'message' has been removed.
Remove support for eslint <7, because the ESLint class does not exist for eslint <7.
Was generated with outdated npm version
@yassin-kammoun-sonarsource
Copy link
Contributor Author

I don't have permission to commit changes on release.yml, and here in particular:

package.json Outdated
@@ -30,31 +30,32 @@
"check-format": "prettier --list-different \"{src,tests}/**/*.ts\""
},
"peerDependencies": {
"eslint": "^5.0.0 || ^6.0.0 || ^7.0.0"
"eslint": "^8.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we keep compatibility with other older versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I tried using our plugin with eslint 5.16.0 and I got some type errors on linting. Not sure how I managed to do that, but I am no longer able to reproduce them. I will revert that change.

ruling/index.ts Outdated
import * as lodash from "lodash";
import * as minimist from "minimist";

const rulesPath = path.join(__dirname, "../lib/rules");

run();
run().catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this catch? won't it throw anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that; I would say it was added for debugging purpose. I will remove it.

type: 'suggestion',
docs: {
description: 'Cognitive Complexity of functions should not be too high',
category: 'Best Practices',
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this go away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dropped (see eslint/eslint#13398)

@@ -30,31 +30,32 @@
"check-format": "prettier --list-different \"{src,tests}/**/*.ts\""
},
"peerDependencies": {
"eslint": "^5.0.0 || ^6.0.0 || ^7.0.0"
"eslint": "^5.0.0 || ^6.0.0 || ^7.0.0|| ^8.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

package-lock should be updated

context.report({ node, message: fileComplexity.toString() });
context.report({
node,
messageId: 'refactorFunction',
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use another message id here, to have final message to contain only fileComplexity value (https://github.com/SonarSource/SonarJS/blob/master/eslint-bridge/src/linter.ts#L389)

@vilchik-elena
Copy link
Contributor

@yassin-kammoun-sonarsource I am not satisfied with the way sonar-runtime messages are done now, but I don't find a better way so far :(
So please have a look at the changes I did (should be minor) and I suggest we test this build in SonarJS before doing the merge of this PR

@@ -650,7 +650,7 @@ class TopLevel {
}
`,
options: [0, 'metric'],
errors: [complexity(25)],
errors: [{ messageId: 'fileComplexity', data: { complexityAmount: complexity.toString() } }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vilchik-elena Something is fishy here: we are calling toString on a function symbol.

@sonarcloud
Copy link

sonarcloud bot commented Nov 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.8% 97.8% Coverage
0.0% 0.0% Duplication

@vilchik-elena vilchik-elena merged commit d5ec6e1 into master Nov 30, 2021
@vilchik-elena vilchik-elena deleted the issue-286 branch November 30, 2021 15:29
@vilchik-elena vilchik-elena added this to the 0.11.0 milestone Nov 30, 2021
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.

Support ESLint 8.x
3 participants