Skip to content

feat(KEY_CODES): warn about KEY_CODES removal #210

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

Merged
merged 5 commits into from
Jan 17, 2023
Merged

feat(KEY_CODES): warn about KEY_CODES removal #210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9778d1
feat(KEY_CODES): warn about KEY_CODES removal
wise-king-sullyman Jan 13, 2023
39f1b97
chore(KEY_CODES): remove unneeded fixable line
wise-king-sullyman Jan 16, 2023
f394f16
fix(KEY_CODES): fix bug causing the warning to throw on all imports
wise-king-sullyman Jan 16, 2023
151525e
feat(CLI): add CLI option to prevent cache creation
wise-king-sullyman Jan 16, 2023
2136162
chore(rules): remove warning rule setting
wise-king-sullyman Jan 16, 2023
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
6 changes: 4 additions & 2 deletions packages/eslint-plugin-pf-codemods/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,13 @@ const rules = {
"tooltip-remove-props": require('./lib/rules/v5/tooltip-remove-props'),
"remove-removeFindDomNode": require('./lib/rules/v5/remove-removeFindDomNode'),
"popover-remove-props": require('./lib/rules/v5/popover-remove-props'),
"warn-key-codes-removed": require('./lib/rules/v5/warn-key-codes-removed'),
};

module.exports = {
configs: {
recommended: {
plugins: ["pf-codemods"],
plugins: ["@patternfly/pf-codemods"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes an error introduced when we scoped the packages under @patternfly

env: {
browser: true,
node: true,
Expand All @@ -83,7 +84,8 @@ module.exports = {
},
},
rules: Object.keys(rules).reduce((acc, rule) => {
acc[`pf-codemods/${rule}`] = "error";
const severity = rule.includes('warn') ? 'warn' : 'error';
acc[`@patternfly/pf-codemods/${rule}`] = severity;
Copy link
Collaborator Author

@wise-king-sullyman wise-king-sullyman Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This allows us to set rules as warnings on the eslint-plugin-pf-codemods side of things.

Alternatively we could set it in the eslintrc file in pf-codemods, not sure which approach would be better and fine with either, but I lean slightly towards this so that it's a warning regardless of if the pf-codemods package is being used or the eslint-plugin-pf-codemods package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've since removed this as it seems some more discussion probably needs to happen around how we'd want this implemented, or if we even actually want it implemented.

return acc;
}, {}),
},
Expand Down
23 changes: 23 additions & 0 deletions packages/eslint-plugin-pf-codemods/lib/rules/v5/warn-key-codes-removed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const { getPackageImports } = require("../../helpers");

// https://github.com/patternfly/patternfly-react/pull/8174
module.exports = {
meta: { fixable: "code" },
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be here if we aren't doing a code fix? also, did you mean this rule to add a warning vs error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, probably not? I'll try removing and see what happens.

And yes, the intent was for it to be a warning. If we want it to be an error though I'm fine with that.

create: function (context) {
const imports = getPackageImports(context, "@patternfly/react-core").filter(
(specifier) => specifier.imported.name === "KEY_CODES"
);

return imports.length == 0
? {}
: {
ImportDeclaration(node) {
context.report({
node,
message:
"The KEY_CODES constant has been removed. We suggest refactoring to use the KeyTypes constant instead.",
});
},
};
},
};
23 changes: 23 additions & 0 deletions packages/eslint-plugin-pf-codemods/test/rules/v5/warn-key-codes-removed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const ruleTester = require("../../ruletester");
const rule = require("../../../lib/rules/v5/warn-key-codes-removed");

ruleTester.run("warn-key-codes-removed", rule, {
valid: [
{
// No @patternfly/react-core import
code: `KEY_CODES`,
},
],
invalid: [
{
code: `import { KEY_CODES } from '@patternfly/react-core';`,
output: `import { KEY_CODES } from '@patternfly/react-core';`,
errors: [
{
message: `The KEY_CODES constant has been removed. We suggest refactoring to use the KeyTypes constant instead.`,
type: "ImportDeclaration",
},
],
},
],
});
6 changes: 6 additions & 0 deletions packages/pf-codemods/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,3 +421,9 @@ Out:
```jsx
<Tooltip />
```

### warn-key-codes-removed [(#8174)](https://github.com/patternfly/patternfly-react/pull/8174)

We've removed the `KEY_CODES` constant from our constants file. If your code relies on it we suggest that you refactor to use `KeyTypes` as `KeyboardEvent.keyCode` is deprecated.

This rule will raise a warning when `KEY_CODES` is imported in a file, but it will not make any code changes.
5 changes: 1 addition & 4 deletions packages/pf-codemods/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env node
const fspath = require('path');
const { CLIEngine } = require('eslint/lib/cli-engine');
const { configs } = require('eslint-plugin-pf-codemods');
const { configs } = require('@patternfly/eslint-plugin-pf-codemods');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another fix for the move to the @patternfly scope.

const { Command } = require('commander');
const program = new Command();

Expand Down Expand Up @@ -34,9 +34,6 @@ function printResults(engine, results, format) {
return false;
}

// Don't show warnings
results.forEach(result => result.messages = result.messages.filter(message => message.severity === 2));

Comment on lines -37 to -39
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This setting was making it so that warnings never surfaced before, it has to be removed so that we can issue them.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you think of a reason this was here in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not off the top of my head, and it was just added in this commit that changed things all over the repo 1ab03a0#diff-06072659dcb0ba81f72d49d18cc77c030ba13d5edacde533742c0dd8ac81730f

const output = formatter(results, {
get rulesMeta() {
if (!rulesMeta) {
Expand Down
5 changes: 0 additions & 5 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,11 +954,6 @@
dependencies:
"@types/node" ">= 8"

"@patternfly/eslint-plugin-pf-codemods@^1.9.2":
version "1.9.2"
resolved "https://registry.yarnpkg.com/@patternfly/eslint-plugin-pf-codemods/-/eslint-plugin-pf-codemods-1.9.2.tgz#10bdf1d513c65f47bcaf5131fbf17318fa1809c7"
integrity sha512-+CtjBJ3SUP0LWLeKaBxGlHmWCpVVoIH5LKeCYeEvXYbOvQ246EhWAjx3TZbOx5V8rR1blc2Tvw+ffBl49yRY3g==

"@types/color-name@^1.1.1":
version "1.1.1"
resolved "https://registry.yarnpkg.com/@types/color-name/-/color-name-1.1.1.tgz#1c1261bbeaa10a8055bbc5d8ab84b7b2afc846a0"
Expand Down