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

Updated eslint-plugin-sonarjs #178

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

jpolavar
Copy link
Contributor

Closes #169

@jpolavar jpolavar added the PATCH label Oct 24, 2024
@jpolavar jpolavar self-assigned this Oct 24, 2024
package.json Outdated
@@ -59,7 +59,7 @@
"eslint-plugin-n": "17.11.1",
"eslint-plugin-no-only-tests": "3.3.0",
"eslint-plugin-no-secrets": "1.0.2",
"eslint-plugin-sonarjs": "1.0.4",
"eslint-plugin-sonarjs": "^2.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

just noticed we have a ^ on this and other peer deps, everything should be fixed to a specific version

@jpolavar jpolavar requested a review from carlansley October 24, 2024 18:43
@le-cong
Copy link
Contributor

le-cong commented Oct 29, 2024

is it possible to have a rough list of significant changes, e.g. it seems require every file to have a peer test file which might not be the case for us.

@jpolavar
Copy link
Contributor Author

is it possible to have a rough list of significant changes, e.g. it seems require every file to have a peer test file which might not be the case for us.

It looks like test files should contain at least one test case, https://sonarsource.github.io/rspec/#/rspec/S2187/javascript

@carlansley
Copy link
Contributor

@le-cong @jpolavar we should update the sonarjs plugin soon, I noticed we're getting quite far behind the current version when I was doing #195. Could we resolve these conflicts and get a new beta out there?

Copy link
Contributor

@le-cong le-cong left a comment

Choose a reason for hiding this comment

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

just a few questions

index.mjs Outdated
@@ -182,6 +182,8 @@ const tsConfigurations = [
// typeof any === "evil".
'@typescript-eslint/no-explicit-any': 'error',

'sonarjs/no-empty-function': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder why these 2 rules are disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like v3 doesn't worry about about empty function, unused expression and this was earlier added to handle eslint error in index.ts

package.json Outdated
@@ -60,7 +60,7 @@
"eslint-plugin-n": "17.15.1",
"eslint-plugin-no-only-tests": "3.3.0",
"eslint-plugin-no-secrets": "2.1.1",
"eslint-plugin-sonarjs": "1.0.4",
"eslint-plugin-sonarjs": "2.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we want to pin v2 or upgrade to v3

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 will upgrade to v3. when i started , we have only v2

@jpolavar jpolavar requested a review from le-cong January 2, 2025 18:59
Copy link

github-actions bot commented Jan 2, 2025

Beta Published - Install Command: npm install @checkdigit/[email protected]

Copy link
Contributor

@le-cong le-cong left a comment

Choose a reason for hiding this comment

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

just a quick comment

@@ -15,14 +16,16 @@ function testHello(_?: string): number {

[].forEach((_lib) => {
// do nothing
// eslint-disable-next-line @typescript-eslint/no-shadow,sonarjs/prefer-immediate-return,eqeqeq
// eslint-disable-next-line @typescript-eslint/no-shadow,eqeqeq
Copy link
Contributor

Choose a reason for hiding this comment

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

is this eqeqeq typo?

Copy link
Contributor Author

@jpolavar jpolavar Jan 2, 2025

Choose a reason for hiding this comment

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

It was added earlier as part of https://eslint.org/docs/latest/rules/eqeqeq to disable rule

@jpolavar jpolavar requested a review from le-cong January 2, 2025 23:21
Copy link

github-actions bot commented Jan 3, 2025

❌ PR review status - has 1 reviewer outstanding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update eslint-plugin-sonarjs
3 participants