feat: add ESLint rule to enforce named ILogger conventions#17687
feat: add ESLint rule to enforce named ILogger conventions#17687ankitsharma101 wants to merge 2 commits into
Conversation
347d1fa to
a985fdc
Compare
sdirix
left a comment
There was a problem hiding this comment.
Looks pretty good already! However we can only activate this if the rest of the codebase is migrated in a separate PR or within this PR as otherwise we have a lot of lint errors.
| "runtime-import-check": require('./rules/runtime-import-check'), | ||
| "shared-dependencies": require('./rules/shared-dependencies') | ||
| "shared-dependencies": require('./rules/shared-dependencies'), | ||
| "named-logger-check": require('./rules/named-logger-check') |
There was a problem hiding this comment.
Registering the rule here only exposes it under @theia/named-logger-check, it doesn't activate it. The rule isn't added to configs/errors.eslintrc.json (or any other config), so npm run lint never runs it and the "How to test" steps in the PR description won't produce an error.
| @@ -0,0 +1,90 @@ | |||
| /** @type {import('eslint').Rule.RuleModule} */ | |||
There was a problem hiding this comment.
Missing the EPL license header and // @ts-check. Every other rule file has both, e.g. annotation-check.js. annotation-check.js also uses proper @typedef imports instead of the inline /** @type {any} */ casts here, worth following the same pattern.
| @@ -0,0 +1,70 @@ | |||
| const { RuleTester } = require('eslint'); | |||
There was a problem hiding this comment.
Missing the EPL license header here too.
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: 'Enforce ILogger usage and naming conventions in @injectable classes', |
There was a problem hiding this comment.
New rule isn't documented in the plugin README, which lists every other rule. Please add a named-logger-check entry.
|
Hi Stefan, thanks for the review! I've updated the rule with the requested license headers, added // @ts-check with proper JSDoc typedefs to satisfy type checking, and added documentation to the README. The rule remains disabled in errors.eslintrc.json as discussed to avoid build breaks during the migration. Ready for another look! |
What it does
Resolves #17468.
This PR introduces a new custom ESLint rule (
named-logger-check.js) to theprivate-eslint-pluginto enforce the new logger coding guidelines. Specifically, the rule ensures that:console.*statements are not used inside@injectable()classes.ILoggerinstances are accompanied by an@named(...)decorator.@namedfollows the required naming convention:[optional-purpose]package-name:class-name#optional-suffix.electron-mainare explicitly excluded from this check, as the DI container is not fully available during early bootstrap.How to test
dev-packages/private-eslint-pluginto verify the AST logic and regex patterns.console.log("test")inside any@injectableclass in the frontend (e.g.,packages/core/src/browser/application-shell.ts). Runyarn lint packages/coreand observe the custom ESLint errorUse injected ILogger instead of console statements in @injectable classes.Follow-ups
Aligning the rest of the codebase to pass this new rule. (Note: Existing violations are currently being addressed in PR #17541).
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers