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

chore(eslint): use @typescript-eslint/stylistic rules #2821

Merged
merged 11 commits into from
Aug 29, 2023

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Aug 25, 2023

  • Introduced the plugin:@typescript-eslint/stylistic rules. This addition ensures that the code follows consistent and recommended coding practices.

  • Addressed linting issues identified by plugin:@typescript-eslint/stylistic.

ℹ️ See configs/stylistic.ts for the exact contents of this config.

Covers #2742

@csouchet csouchet added chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) refactoring Code refactoring labels Aug 25, 2023
@github-actions
Copy link

github-actions bot commented Aug 25, 2023

♻️ PR Preview 9b01ca9 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@github-actions
Copy link

github-actions bot commented Aug 25, 2023

♻️ PR Preview 9b01ca9 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@csouchet csouchet changed the title chore(eslint): enhancing TypeScript ESLint rules and lint fixes chore(eslint): use plugin:@typescript-eslint/stylistic rules Aug 25, 2023
@csouchet csouchet marked this pull request as ready for review August 25, 2023 13:22
@tbouffard tbouffard changed the title chore(eslint): use plugin:@typescript-eslint/stylistic rules chore(eslint): use @typescript-eslint/stylistic rules Aug 25, 2023
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

src/component/options.ts Outdated Show resolved Hide resolved
src/component/registry/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

👍🏿 Good points:

  • consistent syntax when casting and for declaring arrays
  • the syntax when declaring a Map is clearer

🤔 The transition from interfaces to types in the public API can also introduce breaks, in the same way as if we decide to use only interfaces.
Let's discuss this in person.

src/component/mxgraph/initializer.ts Outdated Show resolved Hide resolved
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

We still have to discuss the impact of switching from interface to type for the part of the public API.
Otherwise, LGTM.

test/integration/cross.css.and.style.api.test.ts Outdated Show resolved Hide resolved
@csouchet csouchet marked this pull request as draft August 28, 2023 12:49
…e 'type' and not 'interface'

(cherry picked from commit 78bcbd5)
(cherry picked from commit 26ad0a6)
(cherry picked from commit 43086bf)
(cherry picked from commit be829fa56be1867a3bbd479c219d5af12d069897)
@csouchet
Copy link
Member Author

As decided with @tbouffard, I disabled the @typescript-eslint/consistent-type-definitions rule.
We will choose later if we want to enable it.

@csouchet csouchet marked this pull request as ready for review August 29, 2023 08:56
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -63,6 +64,7 @@ module.exports = {
},
],
'@typescript-eslint/consistent-type-imports': ['error'],
'@typescript-eslint/consistent-type-definitions': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: we may add a comment to explain why we disable it for know. Adding a link to the PR comment is enough IMHO

@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

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.6% 97.6% Coverage
0.0% 0.0% Duplication

@csouchet csouchet merged commit 792e4d1 into master Aug 29, 2023
26 checks passed
@csouchet csouchet deleted the 2742-use_stylistic_rule branch August 29, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) decision record refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants