From 2d2a870fbd104cee453b8d5c86c2447821f692a5 Mon Sep 17 00:00:00 2001 From: Eric Olkowski Date: Thu, 22 Aug 2024 13:27:13 -0400 Subject: [PATCH] Pr feedback --- .../rules/helpers/getLocalComponentName.ts | 8 ++-- .../rules/helpers/getNodeForAttributeFixer.ts | 47 ++++++++++--------- .../rules/helpers/getSpecifierFromImports.ts | 10 ++-- .../removePropertiesFromObjectExpression.ts | 2 +- .../card-updated-clickable-markup.test.ts | 30 ++++++------ .../card-updated-clickable-markup.ts | 12 +++-- 6 files changed, 55 insertions(+), 54 deletions(-) diff --git a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getLocalComponentName.ts b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getLocalComponentName.ts index b0e65492..643855a2 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getLocalComponentName.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getLocalComponentName.ts @@ -1,18 +1,16 @@ -import { ImportSpecifier, ImportDefaultSpecifier } from "estree-jsx"; +import { ImportSpecifier } from "estree-jsx"; import { getSpecifierFromImports } from "./getSpecifierFromImports"; /** Resolves the local name of an import */ export function getLocalComponentName( - namedImports: (ImportSpecifier | ImportDefaultSpecifier)[], + namedImports: ImportSpecifier[], importedName: string ) { const componentImport = getSpecifierFromImports(namedImports, importedName); - const isDefaultImport = componentImport?.type === "ImportDefaultSpecifier"; const isAlias = - !isDefaultImport && componentImport?.imported.name !== componentImport?.local.name; - if ((componentImport && isAlias) || isDefaultImport) { + if (componentImport && isAlias) { return componentImport.local.name; } diff --git a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getNodeForAttributeFixer.ts b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getNodeForAttributeFixer.ts index a55ecd0d..2c72749d 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getNodeForAttributeFixer.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getNodeForAttributeFixer.ts @@ -10,32 +10,37 @@ export function getNodeForAttributeFixer( context: Rule.RuleContext, attribute: JSXAttribute ) { - if (!attribute.value) { + if (!attribute?.value) { return; } - if ( - attribute.value.type === "JSXExpressionContainer" && - attribute.value.expression.type === "Identifier" - ) { - const scope = context.getSourceCode().getScope(attribute); - const variableDeclaration = getVariableDeclaration( - attribute.value.expression.name, - scope - ); - - return variableDeclaration && variableDeclaration.defs[0].node.init; + switch (attribute.value.type) { + case "Literal": + return attribute.value; + case "JSXExpressionContainer": + return getJSXExpressionContainerValue(context, attribute); } +} - if (attribute.value.type === "Literal") { - return attribute.value; +function getJSXExpressionContainerValue( + context: Rule.RuleContext, + attribute: JSXAttribute +) { + if (!attribute.value || attribute.value.type !== "JSXExpressionContainer") { + return; } - if ( - attribute.value.type === "JSXExpressionContainer" && - ["ObjectExpression", "MemberExpression"].includes( - attribute.value.expression.type - ) - ) { - return attribute.value.expression; + + switch (attribute.value.expression.type) { + case "ObjectExpression": + case "MemberExpression": + return attribute.value.expression; + case "Identifier": + const scope = context.getSourceCode().getScope(attribute); + const variableDeclaration = getVariableDeclaration( + attribute.value.expression.name, + scope + ); + + return variableDeclaration && variableDeclaration.defs[0].node.init; } } diff --git a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getSpecifierFromImports.ts b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getSpecifierFromImports.ts index 83ab9d75..7d4693e7 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/helpers/getSpecifierFromImports.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/helpers/getSpecifierFromImports.ts @@ -1,15 +1,13 @@ -import { ImportSpecifier, ImportDefaultSpecifier } from "estree-jsx"; +import { ImportSpecifier } from "estree-jsx"; /** Can be used to extract a specific specifier from an array of imports, such as to only * run logic if X and Y imports are present or to use the specifier properties in other logic. */ export function getSpecifierFromImports( - imports: (ImportSpecifier | ImportDefaultSpecifier)[], + imports: ImportSpecifier[], importedName: string ) { - const importSpecifier = imports.find((imp) => - imp.type === "ImportDefaultSpecifier" - ? imp.local.name === importedName - : imp.imported.name === importedName + const importSpecifier = imports.find( + (imp) => imp.imported.name === importedName ); return importSpecifier; diff --git a/packages/eslint-plugin-pf-codemods/src/rules/helpers/removePropertiesFromObjectExpression.ts b/packages/eslint-plugin-pf-codemods/src/rules/helpers/removePropertiesFromObjectExpression.ts index 28822924..91010a35 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/helpers/removePropertiesFromObjectExpression.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/helpers/removePropertiesFromObjectExpression.ts @@ -32,7 +32,7 @@ export function removePropertiesFromObjectExpression( } if (property.key.type === "Literal") { - return propertyNamesToRemove.includes(property.key.value); + return !propertyNamesToRemove.includes(property.key.value); } return false; diff --git a/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.test.ts b/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.test.ts index 01765df2..10f32858 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.test.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.test.ts @@ -1,6 +1,10 @@ const ruleTester = require("../../ruletester"); import * as rule from "./card-updated-clickable-markup"; +const baseMessage = "The markup for clickable-only cards has been updated."; +const propertyMessage = + " Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards."; + ruleTester.run("card-updated-clickable-markup", rule, { valid: [ { @@ -26,7 +30,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; `, errors: [ { - message: "The markup for clickable-only cards has been updated.", + message: baseMessage, type: "JSXElement", }, ], @@ -37,8 +41,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; `, errors: [ { - message: - "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", + message: baseMessage + propertyMessage, type: "JSXElement", }, ], @@ -49,8 +52,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; `, errors: [ { - message: - "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", + message: baseMessage + propertyMessage, type: "JSXElement", }, ], @@ -61,9 +63,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; `, errors: [ { - message: - "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", - // message: `The markup for clickable-only cards has been updated, now using button and anchor elements for the respective clickable action. The \`selectableActions.selectableActionId\` and \`selectableActions.name\` props are also no longer necessary for clickable-only cards. Passing them in will not cause any errors, but running the fix for this rule will remove them.`, + message: baseMessage + propertyMessage, type: "JSXElement", }, ], @@ -74,8 +74,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; const obj = {}; `, errors: [ { - message: - "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", + message: baseMessage + propertyMessage, type: "JSXElement", }, ], @@ -86,8 +85,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core'; const obj = {to: "#", extra: "thing"}; `, errors: [ { - message: - "The markup for clickable-only cards has been updated.Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards.", + message: baseMessage + propertyMessage, type: "JSXElement", }, ], @@ -98,7 +96,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card as CustomCard, CardHeader as CustomCardHeader } from '@patternfly/react-core'; `, errors: [ { - message: "The markup for clickable-only cards has been updated.", + message: baseMessage, type: "JSXElement", }, ], @@ -109,7 +107,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core/dist/esm/components/Card/index.js'; `, errors: [ { - message: "The markup for clickable-only cards has been updated.", + message: baseMessage, type: "JSXElement", }, ], @@ -119,7 +117,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core/dist/js/components/Card/index.js'; `, errors: [ { - message: "The markup for clickable-only cards has been updated.", + message: baseMessage, type: "JSXElement", }, ], @@ -129,7 +127,7 @@ ruleTester.run("card-updated-clickable-markup", rule, { output: `import { Card, CardHeader } from '@patternfly/react-core/dist/dynamic/components/Card/index.js'; `, errors: [ { - message: "The markup for clickable-only cards has been updated.", + message: baseMessage, type: "JSXElement", }, ], diff --git a/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts b/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts index c440f4e8..5940c269 100644 --- a/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts +++ b/packages/eslint-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts @@ -2,6 +2,7 @@ import { Rule } from "eslint"; import { JSXElement, Property, Literal } from "estree-jsx"; import { getAllImportsFromPackage, + getFromPackage, checkMatchingJSXOpeningElement, getAttribute, getAttributeValue, @@ -17,10 +18,7 @@ module.exports = { meta: { fixable: "code" }, create: function (context: Rule.RuleContext) { const basePackage = "@patternfly/react-core"; - const componentImports = getAllImportsFromPackage(context, basePackage, [ - "Card", - "CardHeader", - ]); + const { imports: componentImports } = getFromPackage(context, basePackage); const cardImport = getSpecifierFromImports(componentImports, "Card"); const cardHeaderImport = getSpecifierFromImports( componentImports, @@ -57,6 +55,10 @@ module.exports = { context, selectableActionsProp.value ); + if (!selectableActionsValue) { + return; + } + const nameProperty = getObjectProperty( context, selectableActionsValue, @@ -72,7 +74,7 @@ module.exports = { "The markup for clickable-only cards has been updated."; const message = `${baseMessage}${ nameProperty || idProperty - ? "Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards." + ? " Additionally, the `selectableActions.selectableActionId` and `selectableActions.name` props are no longer necessary to pass to CardHeader for clickable-only cards." : "" }`; context.report({