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

[New] add jsx-props-no-spread-multi #3724

Merged

Conversation

SimonSchick
Copy link
Contributor

@SimonSchick SimonSchick commented Apr 3, 2024

Please see notes in code.

I think this rule has value in catching some niche bugs, I personally only saw this ~3-4 times throughout my career.

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (393bfa2) to head (57bb4f3).

Current head 57bb4f3 differs from pull request most recent head eafa9e9

Please upload reports for the commit eafa9e9 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
+ Coverage   94.28%   97.75%   +3.46%     
==========================================
  Files         134      135       +1     
  Lines        9613     9630      +17     
  Branches     3486     3490       +4     
==========================================
+ Hits         9064     9414     +350     
+ Misses        549      216     -333     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

configs/recommended.js Outdated Show resolved Hide resolved
docs/rules/jsx-props-no-spread-multi.md Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spread-multi.js Outdated Show resolved Hide resolved
lib/types.d.ts Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spread-multi.js 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.

Adding a new rule has a cost, and if you’ve only seen it 3-4 times (i never have), i don’t think it’ll be that useful. (Generally it’s better to discuss a new rule in an issue first so it doesn’t feel like work is wasted in the event it’s not merged)

@SimonSchick
Copy link
Contributor Author

No hard feelings if it's not merged, I understand it's more of a niche thing, worst case I will port it over to my own plugin :)

@SimonSchick
Copy link
Contributor Author

@ljharb do you have any suggestions where I could sample this rule against a larger set of JSX code in the wild? Github search hasn't been particularly helpful due to the lack of ast node search.

I ran this rule again some internal code bases and found 2 more violations which is still not statistically significant but an indicator that this may be more wide-spread than I thought.

@SimonSchick
Copy link
Contributor Author

Any interest/thoughts on this? If not I will close this PR eow and move into a separate library.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2024

@SimonSchick we use https://www.npmjs.com/package/eslint-remote-tester to test things weekly, that might be worth looking into?

@ljharb ljharb force-pushed the feat/jsx-props-no-spread-multi branch from d366ac0 to e0f2872 Compare June 1, 2024 05:37
@ljharb ljharb marked this pull request as draft June 1, 2024 05:38
@SimonSchick SimonSchick marked this pull request as ready for review June 7, 2024 16:48
@SimonSchick
Copy link
Contributor Author

Updated, but CI seems to be wonky, I rebased ontop of upstream (this repo).

@ljharb ljharb force-pushed the feat/jsx-props-no-spread-multi branch 2 times, most recently from 015d168 to eafa9e9 Compare June 15, 2024 20:22
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.

LGTM. I'm going to hold off merging this until after I cut a release with all the pending patch changes, though.

@ljharb ljharb changed the title feat(rules): add jsx-props-no-spread-multi [New] add jsx-props-no-spread-multi Jun 15, 2024
@ljharb ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25
@ljharb ljharb force-pushed the feat/jsx-props-no-spread-multi branch from eafa9e9 to a762b25 Compare July 13, 2024 21:02
@ljharb ljharb merged commit a762b25 into jsx-eslint:master Jul 15, 2024
330 checks passed
@SimonSchick SimonSchick deleted the feat/jsx-props-no-spread-multi branch July 27, 2024 04:40
@SimonSchick
Copy link
Contributor Author

@ljharb ty for your time and including this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants