-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: data-codemods cleanup rule #745
feat: data-codemods cleanup rule #745
Conversation
...ges/eslint-plugin-pf-codemods/src/rules/v6/dataCodemodsCleanup/data-codemods-cleanup.test.ts
Show resolved
Hide resolved
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.
Looks great! Two small notes/thoughts:
packages/pf-codemods/index.js
Outdated
// data-codemods-cleanup rule won't be included in the recommended rule set | ||
delete configs.recommended.rules[prefix + "data-codemods-cleanup"]; | ||
|
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.
Could you not just add the rule to the betaRules array in the customization file instead of adding logic to this file?
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.
Oh, yeah, I did not read the betaRuleNames description, the name "beta" confused me a bit
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.
FWIW Austin and I had a convo about the naming of the array, though I don't think we settled on whether to change it (or to what if so). Maybe post v6 we can have a brainstorm session about that + anything else we'd like to see changed in the repo
import { | ||
DualListSelector, | ||
LoginMainFooterLinksItem, | ||
MastheadLogo, | ||
} from "@patternfly/react-core"; | ||
|
||
export const DataCodemodsCleanupInput = () => ( | ||
<> | ||
<DualListSelector /> | ||
<LoginMainFooterLinksItem /> | ||
<MastheadLogo /> | ||
</> | ||
); |
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.
Looks like the output file had the formatter ran on it after execution. Doesn't really matter now, but will if we ever implement auto checking of these tsx files in CI.
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.
Oops, I thought we should run prettier on these output files, I do it every time 🤦
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.
👍🏼 other than the above
4c9da6c
to
6a2f521
Compare
Closes #723
Also updates
baseReadme.md
file.