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

feat(linter) eslint: no-new-wrappers #2413

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

alakhpc
Copy link
Contributor

@alakhpc alakhpc commented Feb 15, 2024

Part of: #479

docs: https://eslint.org/docs/latest/rules/no-new-wrappers

I am unsure of what do do with 2 of the eslint tests that use languageOptions: { globals: { String: "off" } } and /* global Boolean:off */.

I've commented them out as of now.

@github-actions github-actions bot added the A-linter Area - Linter label Feb 15, 2024
Copy link

codspeed-hq bot commented Feb 15, 2024

CodSpeed Performance Report

Merging #2413 will improve performances by 13.2%

Comparing alakhpc:eslint-no-new-wrappers (5e7f55d) with main (28ba28f)

Summary

⚡ 7 improvements
✅ 20 untouched benchmarks

Benchmarks breakdown

Benchmark main alakhpc:eslint-no-new-wrappers Change
semantic[antd.js] 818.9 ms 756 ms +8.32%
semantic[cal.com.tsx] 299.1 ms 289.9 ms +3.16%
semantic[checker.ts] 636.5 ms 562.3 ms +13.2%
semantic[pdf.mjs] 151.3 ms 146.3 ms +3.43%
transformer[antd.js] 1.6 s 1.5 s +3.92%
transformer[checker.ts] 1,052.4 ms 975.8 ms +7.85%
transformer[pdf.mjs] 284 ms 274.4 ms +3.47%

Copy link
Collaborator

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

is this the same as https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/unicorn/new_for_builtins.rs

or very similar?

should we merge the two rules.

Thoughts @Boshen ?

@alakhpc
Copy link
Contributor Author

alakhpc commented Feb 15, 2024

That one does seem a lot more comprehensive and should be preferred.

The only reason I can see to include this, is that this falls under correctness while new_for_builtins falls under pedantic

@Boshen
Copy link
Member

Boshen commented Feb 16, 2024

I think we can accept this rule into correctness if we change the help text to indicate why this is correctness.

Who's got the time to read that wall of eslint rule explanation page :thrug:

@Boshen
Copy link
Member

Boshen commented Feb 21, 2024

I changed this to pedantic to be consistent with eslint.

@Boshen Boshen enabled auto-merge (squash) February 21, 2024 06:42
@Boshen Boshen merged commit 9d69167 into oxc-project:main Feb 21, 2024
19 checks passed
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
Part of: oxc-project#479 

docs: https://eslint.org/docs/latest/rules/no-new-wrappers

I am unsure of what do do with 2 of the eslint tests that use
`languageOptions: { globals: { String: "off" } }` and `/* global
Boolean:off */`.

I've commented them out as of now.

---------

Co-authored-by: Boshen <boshenc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants