chore: activate more eslint imports and jsdoc rules#7213
Conversation
Overall package sizeSelf size: 4.39 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7213 +/- ##
=======================================
Coverage 84.55% 84.55%
=======================================
Files 532 532
Lines 22641 22642 +1
=======================================
+ Hits 19144 19145 +1
Misses 3497 3497 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-01-11 15:32:28 Comparing candidate commit 0be0526 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 286 metrics, 34 unstable metrics. |
simon-id
left a comment
simon-id
left a comment
There was a problem hiding this comment.
I looked at appsec files and they looked ok, i assume the changes are ok for other files too
sabrenner
left a comment
sabrenner
left a comment
There was a problem hiding this comment.
lgtm! i just skimmed most of it and mostly just looked at the jsdoc updates, and had a couple questions out of curiosity, although the new types in the jsdocs look correct (the ones my team is codeowner for i had more context on, but some of the other jsdoc changes for other teams look reasonable as well as they seem inferred based on the methods they belong to)
| /** | ||
| * @typedef {Record<string, unknown>} JsonObject | ||
| * | ||
| * @typedef {{ function: { arguments: string } }} ToolCall | ||
| * @typedef {{ content?: string, tool_calls?: ToolCall[] }} ChatDelta | ||
| * | ||
| * @typedef {{ | ||
| * index: number, | ||
| * finish_reason?: string | null, | ||
| * text?: string, | ||
| * delta?: ChatDelta | ||
| * }} StreamChoice | ||
| * | ||
| * @typedef {JsonObject & { choices: StreamChoice[], usage?: unknown }} StreamChunk | ||
| * @typedef {JsonObject & { choices: Array<StreamChoice | undefined>, usage?: unknown }} StreamResponseBody | ||
| * | ||
| * @typedef {JsonObject & { status?: string }} ResponsesApiResponse | ||
| * @typedef {JsonObject & { response?: ResponsesApiResponse }} ResponsesApiChunk | ||
| */ |
There was a problem hiding this comment.
just a question out of curiosity - was this the eslint auto-formatter adding these types as inferred from the method? or did either yourself or an llm write it inferred from the actual method? i think this looks correct though!
There was a problem hiding this comment.
It is inferred by an llm from reading the code
| * @returns {SpanTags} | ||
| */ | ||
| function getSpanTags (ctx) { | ||
| const span = ctx.currentStore?.span | ||
| const carrier = ctx.attributes ?? span?.context()._tags ?? {} | ||
| return carrier | ||
| return /** @type {SpanTags} */ (carrier) |
There was a problem hiding this comment.
also just out of curiosity (possibly just a knowledge gap), do we need both the @returns in the jsdoc and a "typecast" of /** @type {SpanTags} */, or is just the @returns ok?
There was a problem hiding this comment.
ah, is it maybe because the span?.context()._tags might not be typed correctly in its return value, so we want to cast in case?
There was a problem hiding this comment.
Yes, while that actually looks like a bug to cast it. I'll merge it now, but I think we have to continue working on our types anyway a lot, so this could be looked at again a tad later.
3 participants
This sorts the imports (a lot of former code was changed before in that direction) and adds stricter type rules.