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

refactor(migrate/eslint): improve naming-convention migration #2968

Merged
merged 1 commit into from
May 25, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented May 24, 2024

Summary

This PR improves the migration of the naming-convention rule.
It attempts to migrate to the new conventions option of useNamingConvention.

Unfortunately I encounter stack overflow when I am trying to create regexes.
Thus, I commented all codes that compile a regex.
This makes the migration greatly less useful.

I am open to suggestions to solve this issue.

I also fixed a bug in RestrictedRegex that ruled out ([^x]).

Test Plan

I extended a test.

@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages labels May 24, 2024
@Conaclos Conaclos changed the title refcator(migrate/eslint): improve naing-convention migration refcator(migrate/eslint): improve naming-convention migration May 24, 2024
@Conaclos Conaclos changed the title refcator(migrate/eslint): improve naming-convention migration refactor(migrate/eslint): improve naming-convention migration May 24, 2024
@Conaclos Conaclos force-pushed the conaclos/migrate-eslint-conventions branch from cc8cb2f to b2c3ed5 Compare May 24, 2024 12:17
Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 593 593 0
Failed 69 69 0
Panics 0 0 0
Coverage 89.58% 89.58% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13441 13441 0
Failed 4205 4205 0
Panics 0 0 0
Coverage 76.17% 76.17% 0.00%

Copy link

codspeed-hq bot commented May 24, 2024

CodSpeed Performance Report

Merging #2968 will improve performances by 6.49%

Comparing conaclos/migrate-eslint-conventions (4b206ec) with main (08b2013)

Summary

⚡ 1 improvements
✅ 91 untouched benchmarks

Benchmarks breakdown

Benchmark main conaclos/migrate-eslint-conventions Change
jquery.min.js[cached] 28.6 ms 26.8 ms +6.49%

@Conaclos Conaclos force-pushed the conaclos/migrate-eslint-conventions branch from b2c3ed5 to 16d9a31 Compare May 24, 2024 12:56
@ematipico
Copy link
Member

ematipico commented May 24, 2024

Personally, I don't think we should provide a migration path to this rule, because regex coming from eslint are JavaScript regex, and we use a crate that doesn't support JavaScript regex. While this is a best effort work, users should be aware of possible discrepancies.

Personally, it's not worth the trouble, but maybe I'm wrong. I hope I am.

I wished you had discussed this before, but the work is done now.

If you think it's worth the trouble, please make sure that everything is documented on the website, so we set clear expectations.

@Conaclos
Copy link
Member Author

I wished you had discussed this before, but the work is done now.

It was a bullet point in my task list in #2770
I thought everyone expected a migration path for the new option.

Personally, I don't think we should provide a migration path to this rule, because regex coming from eslint are JavaScript regex, and we use a crate that doesn't support JavaScript regex

Yes, however restricted regex should reject regexes that e don't support.

Maybe you are right. This PR already took me too much time. Especially because of the stack overflow.
The resulting migration seems a bit drafty.

@Conaclos Conaclos force-pushed the conaclos/migrate-eslint-conventions branch 4 times, most recently from 953178a to 4b206ec Compare May 25, 2024 15:06
@Conaclos
Copy link
Member Author

I removed the attempt of translating JavaScript regexes into Rust regexes.

@Conaclos Conaclos merged commit b3da3ae into main May 25, 2024
16 checks passed
@Conaclos Conaclos deleted the conaclos/migrate-eslint-conventions branch May 25, 2024 16:12
@Conaclos Conaclos added the A-Changelog Area: changelog label May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants