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] forward-ref-uses-ref: add rule for checking ref parameter is added #3667

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

NotWoods
Copy link
Contributor

Closes #3158

Adds a new rule to require that a second parameter is used with forwardRef. Offers suggestions to add the parameter or remove the forwardRef wrapper.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.76%. Comparing base (597553d) to head (ad8d26e).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3667      +/-   ##
==========================================
+ Coverage   97.62%   97.76%   +0.13%     
==========================================
  Files         132      136       +4     
  Lines        9692     9743      +51     
  Branches     3520     3536      +16     
==========================================
+ Hits         9462     9525      +63     
+ Misses        230      218      -12     

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

configs/recommended.js Outdated Show resolved Hide resolved
lib/rules/forward-ref-uses-ref.js Outdated Show resolved Hide resolved
lib/rules/forward-ref-uses-ref.js Outdated Show resolved Hide resolved
tests/lib/rules/forward-ref-uses-ref.js Outdated Show resolved Hide resolved
tests/lib/rules/forward-ref-uses-ref.js Outdated Show resolved Hide resolved
tests/lib/rules/forward-ref-uses-ref.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.

There’s a few lines that are uncovered; it’d be great to add tests that cover them.

lib/rules/forward-ref-uses-ref.js Outdated Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft December 20, 2023 20:37
@ljharb
Copy link
Member

ljharb commented Dec 21, 2023

(there's still some uncovered lines)

@NotWoods
Copy link
Contributor Author

I'll work on this more in January; currently busy with holidays. Will make sure to get the last few lines covered and see if I can figure out why ESLint 6 & 7 are failing.

@NotWoods NotWoods marked this pull request as ready for review January 24, 2024 23:42
@NotWoods NotWoods requested a review from ljharb January 30, 2024 04:37
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.

There's still a few uncovered added lines.

@ljharb ljharb marked this pull request as draft January 31, 2024 07:54
@NotWoods
Copy link
Contributor Author

I don't see the uncovered lines in Codecov, can you help point them out to me?
image

@NotWoods NotWoods requested a review from ljharb February 27, 2024 04:43
@ljharb
Copy link
Member

ljharb commented Feb 27, 2024

@NotWoods they're right in the "files changed" tab - see https://github.com/jsx-eslint/eslint-plugin-react/pull/3667/files#diff-25114be5f5c5bf7d89a5cc5dfb905ca0d6aafda4a7cb7b49d20df89ceea52bf4R72 for an example

@ljharb
Copy link
Member

ljharb commented Jul 16, 2024

Rebased; tests are failing.

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.

rebased; LGTM assuming tests pass

@ljharb ljharb marked this pull request as ready for review September 11, 2024 20:49
@ljharb ljharb merged commit ceb73fe into jsx-eslint:master Sep 11, 2024
341 of 342 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

rule to ensure forwardRef includes a ref argument.
2 participants