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

[Fix] no-invalid-html-attribute: convert autofix to suggestion #3474

Merged

Conversation

himanshu007-creator
Copy link
Contributor

Fixes : #3458

Fixes jsx-eslint#3458.

Co-authored-by: himanshu007-creator <addyjeridiq@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This seems pretty reasonable - with test changes, and the appropriate descriptions, this should be good to go

lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb 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! Let's update the tests and get this landed :-)

lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
@himanshu007-creator himanshu007-creator marked this pull request as ready for review November 3, 2022 15:47
@ljharb
Copy link
Member

ljharb commented Nov 4, 2022

@himanshu007-creator any test updates? there's a bunch of failing tests :-)

@himanshu007-creator
Copy link
Contributor Author

Hi @ljharb , i would require some help on this

@ljharb
Copy link
Member

ljharb commented Nov 4, 2022

I think what's needed is that all the test cases that were asserting output - ie, an autofixer - instead need to assert suggestions with NO output.

I'm also seeing a syntax error in the logs, which I'll rereview for.

@ljharb
Copy link
Member

ljharb commented Nov 4, 2022

ok, i think this will probably get most of them passing

@ljharb
Copy link
Member

ljharb commented Nov 5, 2022

ok, now the failures are expected :-) all the "output" test cases will need to be updated to not have output, but to have suggestions.

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #3474 (94cf97b) into master (01ab399) will decrease coverage by 3.09%.
The diff coverage is 54.54%.

❗ Current head 94cf97b differs from pull request most recent head a522476. Consider uploading reports for the commit a522476 to get more accurate results

@@            Coverage Diff             @@
##           master    #3474      +/-   ##
==========================================
- Coverage   97.58%   94.48%   -3.10%     
==========================================
  Files         130      129       -1     
  Lines        9218     9189      -29     
  Branches     3341     3333       -8     
==========================================
- Hits         8995     8682     -313     
- Misses        223      507     +284     
Impacted Files Coverage Δ
lib/rules/no-invalid-html-attribute.js 91.94% <54.54%> (-4.77%) ⬇️
lib/util/propTypes.js 68.21% <0.00%> (-29.22%) ⬇️
lib/util/ast.js 73.19% <0.00%> (-23.20%) ⬇️
lib/rules/hook-use-state.js 89.87% <0.00%> (-10.13%) ⬇️
lib/rules/boolean-prop-naming.js 91.56% <0.00%> (-7.84%) ⬇️
lib/rules/no-unused-state.js 93.12% <0.00%> (-6.11%) ⬇️
lib/rules/jsx-props-no-multi-spaces.js 94.11% <0.00%> (-5.89%) ⬇️
lib/rules/no-unused-prop-types.js 91.83% <0.00%> (-4.09%) ⬇️
lib/rules/jsx-no-constructed-context-values.js 91.34% <0.00%> (-1.93%) ⬇️
lib/rules/require-default-props.js 98.38% <0.00%> (-1.62%) ⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2022

Looks better! Seems like for the error cases, each error needs to supply a suggestions array.

@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
@ljharb ljharb force-pushed the suggestion-fix-for-auto-fix branch from 94cf97b to 4f1ace7 Compare November 28, 2022 07:40
@ljharb ljharb changed the title fix: removed autofixer with suggestion [Fix] no-invalid-html-attribute: convert autofix to suggestion Nov 28, 2022
@himanshu007-creator
Copy link
Contributor Author

@ljharb finally tests passed! :)

@ljharb ljharb force-pushed the suggestion-fix-for-auto-fix branch from 4f1ace7 to a522476 Compare November 28, 2022 18:46
@ljharb ljharb merged commit a522476 into jsx-eslint:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

react/no-invalid-html-attribute Shouldn't be auto-fixable
2 participants