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.

Sign up for GitHub

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(generators): made enhancements to generator files #719

Merged
merged 2 commits into from
Aug 6, 2024
Merged

chore(generators): made enhancements to generator files #719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c9d1cf
chore(generators): made enhancements to generator files
thatblindgeye Jul 26, 2024
9268de2
Updated generator for default imports
thatblindgeye Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions generators/src/helpers.js
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions generators/src/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function betterStringSort(a: string, b: string) {
return a.toLowerCase().localeCompare(b.toLowerCase());
}

module.exports = { betterStringSort };
1 change: 1 addition & 0 deletions generators/src/plop-interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export interface Answers {
componentName: string;
propName: string;
ruleName: string;
referenceRepo: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe have this be optional, with it defaulting to pf-react since that is what most of our mods are for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

referencePR: string;
message?: string;
}
16 changes: 14 additions & 2 deletions generators/src/write-readme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@ import { outputFile } from "fs-extra";
import { camelCase } from "case-anything";
import { Answers } from "./plop-interfaces";

async function baseReadme({ referencePR, ruleName, message }: Answers) {
export enum RepoNames {
react = "patternfly-react",
componentGroups = "react-component-groups",
}

async function baseReadme({
referenceRepo,
referencePR,
ruleName,
message,
}: Answers) {
const camelCaseRuleName = camelCase(ruleName);
const readMePath = join(
require
Expand All @@ -15,7 +25,9 @@ async function baseReadme({ referencePR, ruleName, message }: Answers) {
`${ruleName}.md`
);

const readMeContent = `### ${ruleName} [(#${referencePR})](https://github.com/patternfly/patternfly-react/pull/${referencePR})
const prLinkTextPrefix =
referenceRepo === RepoNames.react ? "" : `${referenceRepo}/`;
const readMeContent = `### ${ruleName} [(${prLinkTextPrefix}#${referencePR})](https://github.com/patternfly/${referenceRepo}/pull/${referencePR})

${message}

Expand Down
34 changes: 13 additions & 21 deletions generators/src/write-rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,44 +22,34 @@ export async function genericRule({
componentName,
propName,
ruleName,
referenceRepo,
referencePR,
message,
}: Answers) {
// the formatting for content here looks weird, but that's to preserve indentation in the written file
const content = `import { Rule } from "eslint";
import { JSXOpeningElement } from "estree-jsx";
import { getFromPackage } from "../../helpers";
import { getAllImportsFromPackage, checkMatchingJSXOpeningElement, getAttribute } from "../../helpers";

// https://github.com/patternfly/patternfly-react/pull/${referencePR}
// https://github.com/patternfly/${referenceRepo}/pull/${referencePR}
module.exports = {
meta: { fixable: "code" },
create: function (context: Rule.RuleContext) {
const { imports } = getFromPackage(context, "@patternfly/react-core");

const componentImports = imports.filter(
(specifier) => specifier.imported.name === "${componentName}"
);
const basePackage = "@patternfly/react-core";
const componentImports = getAllImportsFromPackage(context, basePackage, ["${componentName}"]);

return !componentImports.length
? {}
: {
JSXOpeningElement(node: JSXOpeningElement) {
if (
node.name.type === "JSXIdentifier" &&
componentImports
.map((imp) => imp.local.name)
.includes(node.name.name)
) {
const attribute = node.attributes.find(
(attr) =>
attr.type === "JSXAttribute" && attr.name.name === "${propName}"
);
if (attribute) {
if (checkMatchingJSXOpeningElement(node, componentImports)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const ${propName}Prop = getAttribute(node, "${propName}");
if (${propName}Prop) {
context.report({
node,
message: "${message}",
fix(fixer) {
return fixer.replaceText(attribute, "");
return fixer.replaceText(${propName}Prop, "");
},
});
}
Expand All @@ -76,11 +66,12 @@ export async function addEventCBRule({
componentName,
propName,
ruleName,
referenceRepo,
referencePR,
}: Answers) {
const content = `const { addCallbackParam } = require("../../helpers");

// https://github.com/patternfly/patternfly-react/pull/${referencePR}
// https://github.com/patternfly/${referenceRepo}/pull/${referencePR}
module.exports = {
meta: { fixable: "code" },
create: addCallbackParam(["${componentName}"], { ${propName}: "_event" }),
Expand All @@ -93,11 +84,12 @@ export async function swapCBRule({
componentName,
propName,
ruleName,
referenceRepo,
referencePR,
}: Answers) {
const content = `const { addCallbackParam } = require("../../helpers");

// https://github.com/patternfly/patternfly-react/pull/${referencePR}
// https://github.com/patternfly/${referenceRepo}/pull/${referencePR}
module.exports = {
meta: { fixable: "code" },
create: addCallbackParam(["${componentName}"], { ${propName}: { defaultParamName: "_event", previousParamIndex: 1, otherMatchers: /^_?(ev\\w*|e$)/ } }),
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin-pf-codemods/src/rules/helpers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export * from "./includesImport";
export * from "./interfaces";
export * from "./JSXAttributes";
export * from "./JSXElements";
export * from "./nodeMatches";
export * from "./pfPackageMatches";
export * from "./removeElement";
export * from "./removeEmptyLineAfter";
Expand Down
24 changes: 24 additions & 0 deletions packages/eslint-plugin-pf-codemods/src/rules/helpers/nodeMatches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import {
JSXOpeningElement,
ImportSpecifier,
ImportDefaultSpecifier,
} from "estree-jsx";

export function checkMatchingJSXOpeningElement(
node: JSXOpeningElement,
imports:
| ImportSpecifier
| ImportDefaultSpecifier
| (ImportSpecifier | ImportDefaultSpecifier)[]
) {
if (Array.isArray(imports)) {
return (
node.name.type === "JSXIdentifier" &&
imports.map((imp) => imp.local.name).includes(node.name.name)
);
}

return (
node.name.type === "JSXIdentifier" && imports.local.name === node.name.name
);
}
7 changes: 2 additions & 5 deletions ...f-codemods/src/rules/v6/accordionItemWarnUpdateMarkup/accordionItem-warn-update-markup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getFromPackage } from "../../helpers";
import { getFromPackage, checkMatchingJSXOpeningElement } from "../../helpers";
import { Rule } from "eslint";
import { JSXOpeningElement } from "estree-jsx";

Expand All @@ -17,10 +17,7 @@ module.exports = {
? {}
: {
JSXOpeningElement(node: JSXOpeningElement) {
if (
node.name.type === "JSXIdentifier" &&
accordionItemImport.local.name === node.name.name
) {
if (checkMatchingJSXOpeningElement(node, accordionItemImport)) {
context.report({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also good either way you want to go about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
message:
Expand Down
147 changes: 66 additions & 81 deletions plopFile.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,49 @@ const {
swapCBTest,
} = require("./generators/dist/js/write-test");
const {
RepoNames,
genericReadme,
addEventCBReadme,
swapCBReadme
swapCBReadme,
} = require("./generators/dist/js/write-readme");
const {
genericTestSingle,
addEventCBTestSingle,
swapCBTestSingle,
} = require("./generators/dist/js/write-test-single");

const componentNamePrompt = {
type: "input",
name: "componentName",
message: "What is the name of the component being changed? (PascalCase)",
};
const propNamePrompt = {
type: "input",
name: "propName",
message: "What is the name of the prop being changed? (camelCase)",
};
const ruleNamePrompt = {
type: "input",
name: "ruleName",
message: "What should the name of this rule be? (kebab-case)",
};
const referenceRepoPrompt = {
type: "list",
name: "referenceRepo",
message: "What is the repo the PR was made in",
choices: Object.values(RepoNames),
};
const referencePrPrompt = {
type: "input",
name: "referencePR",
message: "What is the PR reference number",
};
const messagePrompt = {
type: "input",
name: "message",
message: "What message should the codemod send? (Sentence case)",
};

module.exports = function (plop) {
plop.setActionType("generateRule", async function (answers, config, plop) {
console.log("Generating rule file", answers.ruleName);
Expand Down Expand Up @@ -62,49 +95,32 @@ module.exports = function (plop) {
}
});

plop.setActionType("generateTestSingle", async function (answers, config, plop) {
console.log("Generating tsx files for", answers.ruleName);
switch (config.generatorSelection) {
case "addEventCB":
await addEventCBTestSingle(answers);
break;
case "swapCB":
await swapCBTestSingle(answers);
break;
default:
await genericTestSingle(answers);
plop.setActionType(
"generateTestSingle",
async function (answers, config, plop) {
console.log("Generating tsx files for", answers.ruleName);
switch (config.generatorSelection) {
case "addEventCB":
await addEventCBTestSingle(answers);
break;
case "swapCB":
await swapCBTestSingle(answers);
break;
default:
await genericTestSingle(answers);
}
}
});
);

plop.setGenerator("generic", {
description: "create a generic new rule",
prompts: [
{
type: "input",
name: "componentName",
message:
"What is the name of the component being changed? (PascalCase)",
},
{
type: "input",
name: "propName",
message: "What is the name of the prop being changed? (camelCase)",
},
{
type: "input",
name: "ruleName",
message: "What should the name of this rule be? (kebab-case)",
},
{
type: "input",
name: "referencePR",
message: "What is the PR reference number",
},
{
type: "input",
name: "message",
message: "What message should the codemod send? (Sentence case)",
},
componentNamePrompt,
propNamePrompt,
ruleNamePrompt,
referenceRepoPrompt,
referencePrPrompt,
messagePrompt,
],
actions: [
{
Expand All @@ -125,27 +141,12 @@ module.exports = function (plop) {
plop.setGenerator("add event parameter", {
description: "add an event parameter to the front of a callback",
prompts: [
{
type: "input",
name: "componentName",
message:
"What is the name of the component being changed? (PascalCase)",
},
{
type: "input",
name: "propName",
message: "What is the name of the prop being changed? (camelCase)",
},
{
type: "input",
name: "ruleName",
message: "What should the name of this rule be? (kebab-case)",
},
{
type: "input",
name: "referencePR",
message: "What is the PR reference number",
},
componentNamePrompt,
propNamePrompt,
ruleNamePrompt,
referenceRepoPrompt,
referencePrPrompt,
,
],
actions: [
{
Expand All @@ -170,27 +171,11 @@ module.exports = function (plop) {
plop.setGenerator("swap parameter", {
description: "move the position of a parameter in a callback to the front",
prompts: [
{
type: "input",
name: "componentName",
message:
"What is the name of the component being changed? (PascalCase)",
},
{
type: "input",
name: "propName",
message: "What is the name of the prop being changed? (camelCase)",
},
{
type: "input",
name: "ruleName",
message: "What should the name of this rule be? (kebab-case)",
},
{
type: "input",
name: "referencePR",
message: "What is the PR reference number",
},
componentNamePrompt,
propNamePrompt,
ruleNamePrompt,
referenceRepoPrompt,
referencePrPrompt,
],
actions: [
{
Expand Down
Loading