feat: add ruleCreationMethods rule#2301
Conversation
🦋 Changeset detectedLatest commit: 62e1a89 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| "RuleCreator adds the `docs` property and ensures consistent rule structure across plugins.", | ||
| ], | ||
| suggestions: [ | ||
| "Import RuleCreator and use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", |
There was a problem hiding this comment.
Should we tell them which package to import RuleCreator from? Without that, I'm not sure I'd know what to do to address the violation.
There was a problem hiding this comment.
how about this?
suggestions: [
"Import RuleCreator from `@flint.fyi/core` and instantiate a ruleCreator (e.g. `const ruleCreator = new RuleCreator({ presets: [...] })`).",
"Use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.",
],
michaelfaith
left a comment
There was a problem hiding this comment.
Just a minor wording suggestion (for the suggestion 😅). Otherwise looks good! I'll mark as Approved and you can decide if you like the suggestion or not.
| suggestions: [ | ||
| "Import RuleCreator and use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", | ||
| "Import RuleCreator from `@flint.fyi/core` and instantiate a ruleCreator (e.g. `const ruleCreator = new RuleCreator({ presets: [...] })`).", | ||
| "Use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", |
There was a problem hiding this comment.
Minor suggestion to read a bit more smoothly between the two lines
| "Use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", | |
| "Create an instance of RuleCreator from `@flint.fyi/core` (e.g. `const ruleCreator = new RuleCreator({ presets: [...] })`).", | |
| "And then use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", |
There was a problem hiding this comment.
(apparently you can't provide suggestions across multiple lines, but you get the idea 😅)
There was a problem hiding this comment.
Oh you have to select multiple lines. Which... GitHub sometimes breaks in the UI 🙃
| const propertyAccess = node.expression as AST.PropertyAccessExpression; | ||
| const type = typeChecker.getTypeAtLocation(propertyAccess.expression); | ||
| if ( | ||
| node.expression.kind === SyntaxKind.PropertyAccessExpression && |
There was a problem hiding this comment.
Oh, I was suggesting something like this
const isPropertyAccessExpression = (node: : AST.CallExpression): node is PropertyAccessExpression {
return node.expression.kind === SyntaxKind.PropertyAccessExpression;
}
const isCreateRuleCall = (node: : AST.CallExpression): boolean {
return isPropertyAccessExpression(node) && node.expression.name.text === "createRule";
}But this works fine too. 👍
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
The implementation looks great - I love how clean this is with the shared isLanguageCreateRule helper! 🙌
Just requesting that an .mdx file be added.
| suggestions: [ | ||
| "Import RuleCreator and use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", | ||
| "Import RuleCreator from `@flint.fyi/core` and instantiate a ruleCreator (e.g. `const ruleCreator = new RuleCreator({ presets: [...] })`).", | ||
| "Use `ruleCreator.createRule(language, { ... })` instead of `language.createRule({ ... })`.", |
There was a problem hiding this comment.
Oh you have to select multiple lines. Which... GitHub sometimes breaks in the UI 🙃
lishaduck
left a comment
There was a problem hiding this comment.
Looks good, just a few very minor comments!
| // Use RuleCreator.createRule instead of Language.createRule | ||
| // But this is the original implementation |
There was a problem hiding this comment.
I just wanna say this is 🔥 lol.
Actually, TBH maybe we should ignore this rule for the core package specifically, because it shouldn't be defining any rules? No, that depends on how we structure #2181. Though, having said that, we probably should structure it out-of-core. Ehh, who cares?
packages/site/src/content/docs/rules/flint/ruleCreationMethods.mdx
Outdated
Show resolved
Hide resolved
….mdx Co-authored-by: Eli <88557639+lishaduck@users.noreply.github.com>
|
Deployment failed with the following error: |
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
I don't know what's going on with the deployment, but this is great. Thanks @kovsu! 🙌
PR Checklist
status: accepting prsOverview