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.

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

feat: introduce eslint plugin #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DarkPurple141
Copy link

Hi @danieldelcore this is mk 2!

Copy link
Contributor

@danieldelcore danieldelcore left a comment

Choose a reason for hiding this comment

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

Thanks so much for raising this PR mate, this is awesome stuff!!

Just have a few questions to be sure I fully understand 😄

"name": "eslint-plugin-codemod",
"version": "0.0.1",
"dependencies": {
"eslint-codemod-utils": "latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was latest intentional? Would probably be better to specify the version here unless there's a reason that i'm missing 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I mean it was a bit meh. I'll add a caret range.

},
});

ruleTester.run('jsx/update-prop-name', rule, {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you see jsx/update-prop-name being used? Is this something someone would configure in their eslintrc, run and then remove from the config?

Or maybe would they have multiple configurations setup, one for modal-dialog, one for button etc then they remain in the eslintfile and can be updated as the version changes?

Copy link
Author

Choose a reason for hiding this comment

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

I think yeah it could run alongside / with / in place of / any codemod approach. Maybe a codemod could even be used to update the eslint config around releases. The flexibility of the current config is that it can be used with lots of configs and thus different modules.

The difficult part to figure out is when to turn it off or how to manage ratcheting it.

code: [
// This has an invalid header so is ignored
`/* integrity: codemod-hash-1399692252 */`,
`// TODO codemod-generated-comment signed: codemod-hash-1399692252`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain how the integrity check works?

So if I was to imagine a codemod run on a repo and the diffs raised in a PR, would these be presented as an error? Or can they be merged and subsequent changes to the file would trigger an error?

Copy link
Author

Choose a reason for hiding this comment

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

@danieldelcore I should add a README for these rules sorry mate, will amend.

In essence the idea is that a codemod would add a header comment and generate hashes off the comments it introduces to code. Each of these comments are validated against the header. So it's clear that this were generated by a script.

I thought of multiple different approaches here tho for example:

  • Using a hash of the whole file from the time that the codemod ran and storing it in a header, then recalculating the hash off the whole file minus the hash. Idea here would be as soon as you make a change in this file it will error. IE don't leave the codemod header / make a change without validating that you've acknowledged the codemod ran.
  • Alternatively just erroring or warning off comments generated by codemod.

Or some combination of the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants