feat: No duplicate import specifiers rule #713
Conversation
adamviktora
force-pushed
the
no-duplicate-import-specifiers
branch
from
2aa6710 to
0ade52f
Compare
...-pf-codemods/src/rules/v6/noDuplicateImportSpecifiers/no-duplicate-import-specifiers.test.ts
Show resolved
Hide resolved
...lugin-pf-codemods/src/rules/v6/noDuplicateImportSpecifiers/no-duplicate-import-specifiers.ts
Outdated
Show resolved
Hide resolved
| Content, | ||
| Content, | ||
| Content, |
There was a problem hiding this comment.
Should this unrelated rules output be updated? Or are we keeping the *Output.tsx files as the output of running just the associated rule on the *Input.tsx file?
There was a problem hiding this comment.
That's a good question. I maybe lean a bit more towards having it as the output of running all rules on the file. But I don't know how we approached this in other rules. What is your opinion on this? Also tagging @thatblindgeye
There was a problem hiding this comment.
I lean more towards having it the output of just the associated rule since these files get pulled into the readme as depictions of what each rule will do, and that's at least what I've been doing and what I thought yall were doing.
Something that's been talked about in the past is the idea of compiling all of input/output files together into master input/output files to ensure that rules play nice together, would you feel better about the "just the associated rule" approach if we rolled out something like that?
There was a problem hiding this comment.
I had been going with having each individual Output file only apply fixes for that specific rule. A "master" file of all in/outputs may not be bad, assuming it's more just for our own eyes (and not something we publish in any README or anything). That's basically what we used to do for v5 was the single file where we added everything to ensure all the rules that ran didn't conflict with one another (too badly).
There was a problem hiding this comment.
Yeah in my head at least it would just be for us to look at, and maybe to build a new CI check around 🤔
There was a problem hiding this comment.
A CI check would be less tedious for us to have to constantly check that master file and maybe notice any issues
There was a problem hiding this comment.
I reverted the file so it is not updated by the no-duplicate-imports rule.
We may have the master file + CI check, I don't find it super necessary, but if there will be time for that, why not, created this issue for it.
| code: `import { Button } from "@patternfly/react-core"; | ||
| import { Button } from "@patternfly/react-core"; |
There was a problem hiding this comment.
Could we also add an invalid test where Component is imported from react-core, but also imported from react-core/dist/...
| valid: [ | ||
| { | ||
| // we care only about imports from "@patternfly/react-core" | ||
| code: `import { Button, Button } from "somewhere"`, |
There was a problem hiding this comment.
And maybe a valid test where Component is imported from react-core, but also react-core/[deprecated|next]?
Closes #709