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

Upgrade typescript-eslint #4452

Merged
merged 9 commits into from
Dec 15, 2023
Merged

Upgrade typescript-eslint #4452

merged 9 commits into from
Dec 15, 2023

Conversation

saberduck
Copy link
Contributor

No description provided.

@saberduck saberduck self-assigned this Dec 4, 2023
@saberduck
Copy link
Contributor Author

This is the current status of the ruling differences. @ilia-kebets-sonarsource @vdiez I would appreciate if you could help me with the review. I suggest we split by rules - each person takes one rule (both js and ts)

Screenshot 2023-12-08 at 17 24 34

@ilia-kebets-sonarsource
Copy link
Contributor

ilia-kebets-sonarsource commented Dec 12, 2023

Here are the suspect cases I encountered:

S3863: Imports from the same module should be merged

  • same path flagged twice
    desktop > app/src/ui/index.tsx: line 37 and 44
  • implementation proposal: add secondary locations
    • useful especially when there is more than one secondary: like ag-grid > src/ts/columnController/balancedColumnTreeBuilder.ts lines 11 - 14
  • documentation proposal: maybe add a coding example for cases where you need to combine the export of properties and some default value like : import React, { Component } from React

S2201: Return values from functions without side effects should not be ignored

  • missing valid errors:
    • desktop > app/src/lib/fix-emoji-spacing.ts: line 34: container.offsetHeight.toString()
    • desktop > app/src/lib/source-map-support.ts: line 127: ;(error.stack || '').toString()

S4325: Redundant casts and non-null assertions should be avoided

  • Missing valid errors:
    • ag-grid > src/ts/entities/column.ts: line 227, 243
if (typeof this.colDef.suppressNavigable === 'boolean') {
  return <boolean> this.colDef.suppressNavigable;
if (typeof this.colDef.editable === 'boolean') {
  return <boolean> this.colDef.editable;
  • ag-grid > src/ts/rendering/renderedCell.ts: line 201
} else if (typeof colDef.checkboxSelection === 'boolean') {
  this.usingWrapper = <boolean> colDef.checkboxSelection;
  • ant-design > site/theme/template/IconDisplay/index.tsx: line 87
const categoriesResult = Object.keys(categories)
  .map((key: CategoriesKeys) => {
// ...
<Category
  key={category}
  title={category as CategoriesKeys}
// ...
/>

S3981: Collection size and array length comparisons should make sense

  • implementation proposal: it might be nice to have a secondary location to know what condition or declaration verifies the redundant condition, but lots of work:
    • es5-shim > es5-shim.js: line 1084
        return value !== null
            && typeof value === 'object'
            && typeof value.length === 'number'
            && value.length >= 0

S2260: JS parser failure

  • interesting that this is introduced now. I don't know what to think:
    • jshint > tests/unit/fixtures/emptystmt.js: line 1 ("var;")

@saberduck
Copy link
Contributor Author

Changes for S6582 come from the fact that @typescript-eslint provides services.program also in cases when it was not available before. The rule was doing early exit when services.program was not available. There were also some improvements to the rule logic itself, typescript-eslint/typescript-eslint@02a37c4

@saberduck saberduck changed the title Upgrade typescript-eslint 6.13 Upgrade typescript-eslint Dec 15, 2023
Copy link

import { EncodedMessage } from 'eslint-plugin-sonarjs/lib/utils/locations';
import { isIdentifier } from 'eslint-plugin-sonarjs/lib/utils/nodes';
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason not to use the utils from SonarJS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, we could

Copy link
Contributor

@ilia-kebets-sonarsource ilia-kebets-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants