Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): noInferrableTypes #4083

Merged
merged 1 commit into from Feb 12, 2023
Merged

feat(rome_js_analyze): noInferrableTypes #4083

merged 1 commit into from Feb 12, 2023

Conversation

Conaclos
Copy link
Contributor

@Conaclos Conaclos commented Dec 21, 2022

Summary

Closes #4084

This implements ESLint's no-inferrable-types with some change:

  • property parameters are supported
  • readonly properties are supported
  • we accept wide types in const contexts. This allows to hide information: this is particularly useful when exporting a constant (export const C: number = 1). This also looks more consistent (ESLint allows const x: 1 | 2 = 1. Thus why const x: number = 1 should be rejected?)
  • primitive type constructors are not supported (String(1), Number('1') ...):. They may be shadowed
  • undefined value is not supported (undefined may be shadowed)
  • type RegExp is not supported because it may be shadowed
  • we do not support the options that allow disabling the rule for default parameters and/or properties.

Test Plan

Includes a large set of unit tests.

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Dec 21, 2022

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit ff13139
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63e9753974bdf60008bedfad

@Conaclos Conaclos requested review from ematipico and removed request for xunilrj and leops December 23, 2022 18:15
Comment on lines 80 to 83
i Safe fix: Remove the type annotation.

2 │ const·x:·/*before*/·/*inside*/·1·/*after*/·=·(1);
│ ------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

The fix of the rule can't be flagged as "safe" because it removes trivia, as we can see in this snapshot. If removing trivia is the intended behaviour, then the fix should be Applicability:MaybIncorrect

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 not interventional. I fixed the issue.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good, we can merge once rebased and conflicts are resolved

@Conaclos
Copy link
Contributor Author

Looks good, we can merge once rebased and conflicts are resolved

Done :)

@ematipico ematipico merged commit 7e6710e into rome:main Feb 12, 2023
@Conaclos Conaclos deleted the no_inferrable_types branch March 7, 2023 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noInferrableTypes, no-inferrable-types
2 participants