chore(generators): made enhancements to generator files #719
Conversation
| node.name.type === "JSXIdentifier" && | ||
| accordionItemImport.local.name === node.name.name | ||
| ) { | ||
| if (checkMatchingJSXOpeningElement(node, accordionItemImport)) { |
There was a problem hiding this comment.
Only updated the one file for now. I could update the rest of the files as part of this or we can do that as a followup at some other point, I don't mind either way
There was a problem hiding this comment.
I'm also good either way you want to go about it.
There was a problem hiding this comment.
I don't think we need to refactor our existing codemods, since they are a "one time use" code and we probably won't copy the code from them in the future. So IMO updating the write-rule generator is sufficient.
| node.name.type === "JSXIdentifier" && | ||
| accordionItemImport.local.name === node.name.name | ||
| ) { | ||
| if (checkMatchingJSXOpeningElement(node, accordionItemImport)) { |
There was a problem hiding this comment.
I'm also good either way you want to go about it.
| @@ -2,6 +2,7 @@ export interface Answers { | |||
| componentName: string; | |||
| propName: string; | |||
| ruleName: string; | |||
| referenceRepo: string; | |||
There was a problem hiding this comment.
Should we maybe have this be optional, with it defaulting to pf-react since that is what most of our mods are for?
There was a problem hiding this comment.
@adamviktora wdyt?
For context there was some discussion on Slack:
- My intent was to have the enum sort of be the source of truth for any repo names we create codemods for, with that being the only thing we would have to update (optimistically at least; the prompt creates an array for the values of this enum). It would involve less manually typing an exact repo correctly/ensuring the repo name is exactly correct for the PR url we create for the files
- Alternatively, the repo name prompt becomes a regular input that defaults to
patternfly-react, but have the ability to input custom text if you want
There was a problem hiding this comment.
I like the enum as it is now. When it is on patternfly-react as default, it is just one enter press, same as for the regular input. And I don't think we will need to specify many more custom repos in the future. And if so, there will probably be more codemods for that repo, so that's why I think updating an enum once and then picking an option is easier than always having to write a repo name manually.
| @@ -2,6 +2,7 @@ export interface Answers { | |||
| componentName: string; | |||
| propName: string; | |||
| ruleName: string; | |||
| referenceRepo: string; | |||
There was a problem hiding this comment.
I like the enum as it is now. When it is on patternfly-react as default, it is just one enter press, same as for the regular input. And I don't think we will need to specify many more custom repos in the future. And if so, there will probably be more codemods for that repo, so that's why I think updating an enum once and then picking an option is easier than always having to write a repo name manually.
| node.name.type === "JSXIdentifier" && | ||
| accordionItemImport.local.name === node.name.name | ||
| ) { | ||
| if (checkMatchingJSXOpeningElement(node, accordionItemImport)) { |
There was a problem hiding this comment.
I don't think we need to refactor our existing codemods, since they are a "one time use" code and we probably won't copy the code from them in the future. So IMO updating the write-rule generator is sufficient.
| (attr) => | ||
| attr.type === "JSXAttribute" && attr.name.name === "${propName}" | ||
| ); | ||
| if (checkMatchingJSXOpeningElement(node, componentImports)) { |
There was a problem hiding this comment.
Just a question - do we want to also care about default imports? It was a concern in the react-component-groups repo, where I created the getImportSpecifiersLocalNames helper for that, which was using the getDefaultImportsFromPackage helper inside. But it is rather an exception and most of the rules will still be on the patternfly-react repo where we don't have the default imports. So I am ok with keeping the generic generator like this.
There was a problem hiding this comment.
Yeah it isn't something we need in most cases, but is there any reason we wouldn't want to handle default imports by default though? 🤔
Doesn't have to be part of this PR though.
thatblindgeye
force-pushed
the
generatorEnhance
branch
from
0da0083 to
9268de2
Compare
Closes #720