-
Notifications
You must be signed in to change notification settings - Fork 5
Add missing react- prefix
#23
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
Conversation
3672bf8 to
3a52df5
Compare
| rules: { | ||
| "react-prefer-function-component/prefer-function-component": "error", | ||
| "react-prefer-function-component/react-prefer-function-component": | ||
| "error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to split this into 2 lines to make Prettier happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not seeing this when I run pnpm prettier --write packages/eslint-plugin-react-prefer-function-component/src/config.mts. The repo's lint check should pick up any issues too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't include it initially but that caused the CI Prettier check to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tatethurston because the name uses in the plugin’s rules property doesn’t match what the config references here. Just try linting using the “flat config” example from this repo and you’ll see the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thank you — I totally missed the prefix change and thought this was only a white space change.
I’m going to add a test case for this and I’ll cut a new release this evening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Sorry for missing this in my previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all -- that was a testing gap. I've added a CI check for this. There is probably a more ergonomic solution, but it seems good enough for now:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Published v4.0.1 with your fix
|
I should have the codecov issue fixed now so CI will pass for forked repo PRs. That will just require pulling in |
Yep, rebasing now. I'll also undo the Prettier change and see if it fails again. |
3a52df5 to
20ac242
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 31 31
Branches 11 11
=========================================
Hits 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
20ac242 to
f2b0592
Compare
Follow-up from #20.