-
Notifications
You must be signed in to change notification settings - Fork 596
chore: lint .js and .mjs files (in addition to the existing .ts file linting) #2940
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
chore: lint .js and .mjs files (in addition to the existing .ts file linting) #2940
Conversation
…linting) In open-telemetry#2567 support was added to eslint.config.js for linting .js and .mjs files -- before it only supported .ts files. Originally this was added to support linting examples/... which includes many .js files. This change enables linting of .js and .mjs files in the rest of the repo. Refs: open-telemetry#2891
^^ the first commit is just the package.json changes. Subsequent commits will include lint:fix changes (and if necessary manual changes to for |
After running lint:fix (where 99% of changes were style-only tweaks to .eslintrc.js files), the only lint failures are in instr-aws-lambda:
All are with the Note that our eslint rules for TypeScript test files turn this rule off: opentelemetry-js-contrib/eslint.config.js Line 77 in 74ddb1e
We could do the same for .js files (Option 1), or add a more nuanced config something like this, which supports adding a leading "no-unused-vars": [
"error",
{
"vars": "all",
"args": "all",
"caughtErrors": "all",
"argsIgnorePattern": "^_",
"caughtErrorsIgnorePattern": "^_",
"varsIgnorePattern": "^_"
}
] Anyone have opinions? Update: In commit 092f156 I've done "Option 1". |
…for test .ts files
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2940 +/- ##
=======================================
Coverage 89.55% 89.55%
=======================================
Files 193 193
Lines 9698 9698
Branches 2011 2011
=======================================
Hits 8685 8685
Misses 1013 1013 🚀 New features to boost your workflow:
|
files: ["test/**/*.js", "test/**/*.mjs"], | ||
rules: { | ||
"no-unused-vars": "off", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer note: We turn on the same rule for .ts files in test/... dirs.
See #2940 (comment) for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phew.. a lot of files to look at 😅
thanks for working on this, looks great
In #2567 support
was added to eslint.config.js for linting .js and .mjs files -- before it
only supported .ts files.
Originally this was added to support linting examples/... which includes many
.js files.
This change enables linting of .js and .mjs files in the rest of the repo.
Refs: #2891