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.

Sign up for GitHub

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

Jump to bottom

Add --fix-type support #3165

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Add --fix-type support #3165

wants to merge 7 commits into from

Conversation

sryze
Copy link

@sryze sryze commented Jan 4, 2022 •
edited by ljharb

Add type property to all rules to support usage of eslint's --fix-type option

Fixes #2618. Fixes #2039.

Successor of this PR: #2619

Vlad Esin and others added 6 commits January 5, 2022 03:00
@sryze
Add support --fix-type
7e23376 
1) Add the type field in rule's meta
2) Fix the fixable field
@ljharb @sryze
Apply suggestions from code review
c4cd854 
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
@ljharb @sryze
Apply suggestions from code review
8e8389d 
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
@sryze
Apply suggestions from code review
3de1c8d
@sryze
Update void-dom-elements-no-children.js
381ba11
@sryze
Add type to more rules
3e7ebd5
@sryze
Copy link
Author

sryze commented Jan 4, 2022

Rebased

ljharb
ljharb requested changes Jan 4, 2022
View reviewed changes
lib/rules/jsx-closing-bracket-location.js Outdated
docs: {
description: 'Validate closing bracket location in JSX',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-closing-bracket-location'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

why is this change correct?

(if it is correct, i'd like to land it separately, before this PR)

Copy link
Author

@sryze sryze Jan 4, 2022 •
edited

Choose a reason for hiding this comment

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

it was part of the original PR

I'm not 100% sure what this property means because the documentation doesn't mention what is the difference between code and whitespace; possibly whitepsace means that a fix will only change whitespace characters

Copy link
Author

Choose a reason for hiding this comment

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

@ljharb I've reverted the changes to fixable and category in made by the previous PR author as they are not related to --fix-type (purpose of this PR); please have a look, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

looks like this change was correct, because the fix changes only spacing before the bracket (indents + newline), see eslint/eslint#15489 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@sryze can we make a separate PR that changes all the "code"s to "whitespace"s?

lib/rules/jsx-curly-spacing.js Outdated
docs: {
description: 'Enforce or disallow spaces inside of curly braces in JSX attributes',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-curly-spacing'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

same here?

lib/rules/jsx-equals-spacing.js Outdated
docs: {
description: 'Disallow or enforce spaces around equal signs in JSX attributes',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-equals-spacing'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

and here?

lib/rules/jsx-first-prop-new-line.js Outdated
docs: {
description: 'Ensure proper position of the first property in JSX',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-first-prop-new-line'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

and here

lib/rules/jsx-indent-props.js Outdated
docs: {
description: 'Validate props indentation in JSX',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-indent-props'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

and here

lib/rules/jsx-props-no-multi-spaces.js Outdated
docs: {
description: 'Disallow multiple spaces between inline JSX props',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-props-no-multi-spaces'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

and here

lib/rules/jsx-space-before-closing.js Outdated
deprecated: true,
docs: {
description: 'Validate spacing before closing bracket in JSX',
category: 'Stylistic Issues',
recommended: false,
url: docsUrl('jsx-space-before-closing'),
},
fixable: 'code',
fixable: 'whitespace',
Copy link
Member

Choose a reason for hiding this comment

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

and here

lib/rules/no-array-index-key.js Outdated
Comment on lines 26 to 27
category: 'Best Practices',
category: 'Possible Errors',
Copy link
Member

Choose a reason for hiding this comment

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

changing the category is a breaking change; also, this is indeed a best practice, and does not necessarily indicate a problem.

lib/rules/no-typos.js Outdated
Comment on lines 34 to 35
category: 'Stylistic Issues',
category: 'Possible Errors',
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change, and also should be done in a separate PR.

lib/rules/void-dom-elements-no-children.js Outdated
Comment on lines 54 to 55
category: 'Best Practices',
category: 'Possible Errors',
Copy link
Member

Choose a reason for hiding this comment

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

also here

@ljharb ljharb marked this pull request as draft February 1, 2022 23:54
@ljharb ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45
@ljharb ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Autofix --fix-type layout [ESLint v5.9.0] Fix types
2 participants
@sryze @ljharb