-
Notifications
You must be signed in to change notification settings - Fork 842
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
refactor(otlp-exporter-base): fix eslint warnigns #5391
refactor(otlp-exporter-base): fix eslint warnigns #5391
Conversation
Fix the following eslint warnings: ``` /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts 55:9 warning Forbidden non-null assertion @typescript-eslint/no-non-null-assertion /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts 95:35 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any ``` The first was that we know we already checked and initialized a related variable, but TypeScript has no idea they are related. We can better communicate that, both to TypeScript and to humans, by groupping the related variables into a single one, that is either set or not. The second was a little strange, and I don't really see what the `any` is doing there. `res.on('end')` is already typed to have an `Error` type on the callback argument (maybe that wasn't always the case?). If we trust the type, then there is nothing more we need to do. If we don't trust the type, widening it to `any` without other runtime checks isn't going to do anything for us either. So I just removed it and everything was still fine. Ref open-telemetry#5365
sendWithHttp, | ||
createHttpAgent, | ||
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
} = require('./http-transport-utils'); |
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 should be updated to use dynamic import, but @pichlermarc is working on that elsewhere and needed to change the TS settings first. Conveniently, this is already used in an async context.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5391 +/- ##
=======================================
Coverage 94.64% 94.64%
=======================================
Files 318 318
Lines 8033 8035 +2
Branches 1688 1687 -1
=======================================
+ Hits 7603 7605 +2
Misses 430 430
|
39e9a09
Which problem is this PR solving?
Fix the following eslint warnings:
Ref #5365
Short description of the changes
The first was that we know we already checked and initialized a related variable, but TypeScript has no idea they are related. We can better communicate that, both to TypeScript and to humans, by groupping the related variables into a single one, that is either set or not.
The second was a little strange, and I don't really see what the
any
is doing there.res.on('end')
is already typed to have anError
type on the callback argument (maybe that wasn't always the case?). If we trust the type, then there is nothing more we need to do. If we don't trust the type, widening it toany
without other runtime checks isn't going to do anything for us either. So I just removed it and everything was still fine.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: