-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore(refactor): Refactor rule structure to prevent merge conflicts #565
chore(refactor): Refactor rule structure to prevent merge conflicts #565
Conversation
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.
Really only one thing that needs to be updated (v5 README), otherwise questions/possible updates.
Also as potential followups that we went over during our meet up:
- A mass test file for input/output where all rule example are in a single file (essentially the
test.tsx
file in the test directory. - A table of contents for the rules in the README to more quickly jump to a particular rule (rather than having to scroll/find in the page)
generators/src/write-test-single.ts
Outdated
export async function addEventCBTestSingle(answers: Answers) { | ||
const { componentName, propName } = answers; | ||
|
||
baseTestSingle( | ||
answers, | ||
` <${componentName} ${propName}={foo => handler(foo)} />` | ||
); | ||
} | ||
|
||
export async function swapCBTestSingle(answers: Answers) { | ||
const { componentName, propName } = answers; | ||
|
||
baseTestSingle( | ||
answers, | ||
` <${componentName} ${propName}={(foo, event) => handler(foo, event)} />` | ||
); | ||
} |
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.
Do you think it'd be worth adding the other ways that these rules may make updates? Like if a reference to a function within the same scope is passed to propName instead of an arrow function.
packages/pf-codemods/v5-README.md
Outdated
|
||
Some rules will add either a comment (`/* data-codemods */`) or data attribute (`data-codemods="true"`) in order to prevent certain other rules from applying an unnecessary fix. | ||
|
||
### example-rule[(#1234)](https://github.com/patternfly/patternfly-react/pull/1234) |
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.
This README needs to be fixed to house the v5 content
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.
Whoops
packages/pf-codemods/index.js
Outdated
function ruleVersion(options) { | ||
if (options.v4) { | ||
return [...ruleVersionMapping["v5"], ...ruleVersionMapping["v6"]]; | ||
} else if (options.v6) { | ||
return [...ruleVersionMapping["v4"], ...ruleVersionMapping["v5"]]; | ||
} else { | ||
return [...ruleVersionMapping["v4"], ...ruleVersionMapping["v6"]]; | ||
} | ||
} |
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.
Just to confirm, the point of this is to return the rules thare aren't for the version specified in options? So v4
in options means we want this to return the v5 and v6 rules so that we can remove them?
If so then a slight nit would be the function name; Reading this at first I thought this would be an array of the rules we'd want to run, but further down looks like these are the ones we're excluding.
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.
Yeah that was how I thought it worked initially before as well, I'll see what I can come up with for better name.
packages/pf-codemods/index.js
Outdated
v5Rules, | ||
v6rules, |
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.
Do we need this require
d in here? They're not being used from what I can tell.
console.error("Missing file in rule directory: ", ruleDir); | ||
throw new Error("Missing file in rule directory"); |
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.
Do we need both the console and throw error, or could we just include the ruleDir in the thrown error message?
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.
Hmmmmm probably
Closes #554