-
Notifications
You must be signed in to change notification settings - Fork 309
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
upgrade eslint, eslint-plugin-n, eslint-config-standard (VULN-5530) #4032
Conversation
Overall package sizeSelf size: 5.99 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-02-07 18:16:12 Comparing candidate commit 57c2f6c in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 4 unstable metrics. scenario:plugin-graphql-with-depth-off-18
scenario:plugin-graphql-with-depth-on-max-18
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4032 +/- ##
=======================================
Coverage 85.15% 85.15%
=======================================
Files 243 243
Lines 10504 10504
Branches 33 33
=======================================
Hits 8945 8945
Misses 1559 1559 ☔ View full report in Codecov by Sentry. |
@@ -1,4 +1,5 @@ | |||
const http = require('http') | |||
// eslint-disable-next-line import/no-absolute-path |
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.
If we allow this, why not add it as a global rule?
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.
It's some junk for a test that we otherwise wouldn't want i real tracer code
@@ -3,4 +3,5 @@ require('dd-trace').init({ | |||
service: 'dd-trace-bad-init' | |||
}) | |||
|
|||
// eslint-disable-next-line import/no-extraneous-dependencies |
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.
Similarly, if this is allowed in the project, why not add it as a global rule?
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.
It's some junk for a test that we otherwise wouldn't want i real tracer code
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.
Could always add a .eslintrc override at the root folder of the tests.
"eslint-plugin-import": "^2.8.0", | ||
"eslint-plugin-mocha": "^10.1.0", | ||
"eslint-plugin-n": "^15.7.0", | ||
"eslint-plugin-node": "^5.2.1", | ||
"eslint-plugin-n": "^16.6.2", |
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.
This version doesn't support older versions of Node. Security fixes should be backportable to all release lines.
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.
Hmm... Do we run the linter with every supported version of Node?
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.
In some cases yes.
@@ -54,6 +54,7 @@ class SpanProcessor { | |||
} | |||
|
|||
if (this._killAll) { | |||
// eslint-disable-next-line array-callback-return |
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.
Why is it a .map
then? Similar question for all other occurrences.
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.
Looks like the author intended to use a forEach?
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.
Then I'd say to change the code instead of adding an eslint rule.
do we still want to do this? @tlhunter |
Pull request was closed
What does this PR do?
Motivation