typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

fix(rule-tester): fixes on the same line are not fixed properly#8252

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

StyleShit wants to merge1 commit intotypescript-eslint:mainfromStyleShit:fix/rule-tester-multiple-fixes

Closed

fix(rule-tester): fixes on the same line are not fixed properly#8252

StyleShit wants to merge1 commit intotypescript-eslint:mainfromStyleShit:fix/rule-tester-multiple-fixes

Conversation

Copy link

Contributor

StyleShit commentedJan 14, 2024•
edited
Loading

Closes#8251

fix(rule-tester): fixes on the same line are not fixed properly

f25d901

Copy link

Contributor

typescript-eslintbot commentedJan 14, 2024

Thanks for the PR,@StyleShit!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedJan 14, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`f25d901`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/65a41f6d6b33c90009bbc24f
😎 Deploy Preview	https://deploy-preview-8252--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 3 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

StyleShit mentioned this pull request

Jan 14, 2024

Bug: RuleTester doesn't fix properly when there are multiple fixes on the same line#8251

Closed

4 tasks

StyleShit commented

Jan 14, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/semi.test.ts

		},
		{
		code:'a;\n++b;',
		output:'a\n++b;',

Copy link

ContributorAuthor

StyleShitJan 14, 2024

Choose a reason for hiding this comment

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

This was a bug in the tests. See other tests above.

StyleShit commented

Jan 14, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/space-infix-ops.test.ts

		`,
		output:`
		type Test = \|string \| (((() => void))) \| string;
		type Test = \|string \| (((() => void))) \| string;

Copy link

ContributorAuthor

StyleShitJan 14, 2024•
edited
Loading

Choose a reason for hiding this comment

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

This should be the real fix. Seethis playground.

The test currently fails because it's being fixed to:

typeTest=|string|(((()=>void)))|string;

(notice there are 2 spaces after the=)
The reason is probably because there are 2 fixes that add spaces before and after the relevant characters (= &|), which then results in 2 spaces.

Not sure how to approach this. I have 2 options in mind:

Trimming the spaces after each fix somehow (probably a bad idea)
Re-running the rule tester on the same line after each fix (is that what happens in real life? not sure if/how to do it though)

WDYT?