feat: align recommended ruleset with scanners and improve configuration#164
Conversation
tgrosinger
left a comment
tgrosinger
left a comment
There was a problem hiding this comment.
This pull request seems to be doing a lot of different things all lumped into one commit, making it very hard to understand what is changing and why.
I think we need to break this up into more targeted changes that can be effectively reviewed and reasoned about.
| const manifest = getManifest(); | ||
| const packageJson = JSON.parse(fs.readFileSync(path.join(import.meta.dirname, "../../package.json"), "utf8")) as PackageJson; | ||
|
|
||
| function findPackageJson(startDir: string): string { |
There was a problem hiding this comment.
I encountered Claude trying to add this too and it makes me a little uncomfortable. Let's at the very least not walk higher than the first .git directory sibling we can find.
| }; | ||
|
|
||
| // Combined rules for TypeScript files | ||
| const recommendedPluginRulesConfig: RulesConfig = { |
There was a problem hiding this comment.
Is there a reason for shuffling these around? It feels like the end result is equivalent. We already had a way of only running rules that require type information on TS files, we were just combining them in the rules array below instead of creating a separate RulesConfig.
| // Rules that require TypeScript type information (TypeScript-only). | ||
| // These must only run on files parsed by @typescript-eslint/parser. | ||
| const recommendedPluginRulesConfigTypeChecked: RulesConfig = { | ||
| "obsidianmd/no-plugin-as-component": "error", |
There was a problem hiding this comment.
Please shoot for min-diff when possible. If this array had been left in the same location as the removed recommendedTypedRulesConfig it would be much easier to see which of the internals were actually changed.
| ]); | ||
|
|
||
| // Scanner's restricted imports (same packages, warn severity, scanner's moment phrasing) | ||
| const scannerRestrictedImportsOptions = [ |
There was a problem hiding this comment.
Is this being duplicated? This information is already defined in ruleOptions.ts.
| "eslint-comments": eslintComments, | ||
| obsidianmd: plugin, | ||
| }, | ||
| rules: { |
There was a problem hiding this comment.
Why are all of these being defined here?
Taking one just as an example no-forbidden-elements is already defined as an error in lib/index.ts.
2 participants
Summary
Unifies #86 and #158 into a single changeset. The
recommendedconfig now matches the community plugin scanner's ruleset and properly scopes type-checked rules to TypeScript files only.Supersedes #86 and #158.
Problem
Two issues existed independently:
getParserServices(likeno-plugin-as-component,no-view-references-in-plugin,prefer-file-manager-trash-file,prefer-instanceof,no-unsupported-api) would crash or produce misleading errors when run on plain JavaScript files that aren't parsed by@typescript-eslint/parser.recommendedlocally got different results than during review. The scanner appliestoWarns(), re-escalates ~14 rules back to error, adds rules not in recommended (no-unsanitized/*,@typescript-eslint/no-unsafe-*,eslint-comments/*), and disables rules likevalidate-manifestandui/sentence-case. There was no way to replicate what the scanner checks without reverse-engineering its config chain.Changes
Type-scoped rule split (
lib/index.ts)recommendedPluginRulesConfigintorecommendedPluginRulesConfigBase(JS + TS) andrecommendedPluginRulesConfigTypeChecked(TS only)tseslintinternalsScanner-matching recommended config (
lib/index.ts)recommendednow matches the community plugin scanner's portal-release configno-eval,no-implied-eval,no-unsanitized/method,no-unsanitized/property,no-forbidden-elements,regex-lookbehindsettings-tab/*,detach-leaves,no-plugin-as-component,no-sample-code,platform,no-static-styles-assignment,no-view-references-in-plugin,no-unsupported-api,sample-namesobsidianmdrules downgraded towarn(matching scanner behavior)no-nodejs-modulesalwayswarn(scanner doesn't checkmanifest.isDesktopOnly)validate-manifest,validate-license,ui/sentence-case*,no-undef,no-console,import/no-unresolved,@typescript-eslint/restrict-template-expressions,@typescript-eslint/no-base-to-string@typescript-eslint/no-unsafe-*family atwarnno-new-funcoff, replaced byrule-custom-messagewrapper combiningno-new-funcandno-consolecustom messageseslint-comments/no-unlimited-disable,require-description,disable-enable-pair,no-restricted-disableaterrorlinterOptions.reportUnusedDisableDirectivesandreportUnusedInlineConfigsaterrordepend/ban-dependenciesatwarnfor source filesNew dependency
@eslint-community/eslint-plugin-eslint-commentsas a regular dependency with type shim inlib/shim.d.tsimport.meta.dirnamefixfindPackageJson()helper that walks up directories to findeslint-plugin-obsidianmd'spackage.json, withimport.meta.dirnamefallback for Node 18 compatibilityDocumentation
recommendedmatches the scannereslint-doc-generatorTests
tests/recommendedConfig.test.tswith structural smoke tests verifying the config is well-formed, all registered rules are referenced, type-checked rules are properly scoped, and required plugins are registeredReview feedback addressed
prefer-active-window-timersdoesn't existprefer-window-timersno-global-thisdropped unintentionallyprefer-active-docchanged fromwarntooffunintentionallywarntseslintinternalspackageJsonextraction silently removes from usersflatRecommendedConfigScanner compatibility
Verified against the community plugin scanner. The scanner consumes
obsidianmd.configs.recommendedand applies its own overrides at each config level. Since all overrides are explicit per-rule, they take precedence regardless of whatrecommendedsets. No changes required in the scanner.What changes for consumers
The
recommendedconfig now produces results matching the community plugin scanner instead of stricter standalone rules. Rules that were previouslyerrormay now bewarnoroffto match scanner behavior. Consumers who want stricter checking can override individual rules in their own config.