Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments#10781
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@ronami! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedFeb 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
| /** | ||
| * @returns the first valid JSDoc comment annotating `node` | ||
| */ | ||
| function getJSDocCommentForNode( | ||
| node: TSESTree.Node, | ||
| ): TSESTree.Comment | undefined { | ||
| return context.sourceCode | ||
| .getCommentsBefore(node) | ||
| .reverse() | ||
| .find(comment => isJSDocComment(comment)); | ||
| } |
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 seems only the first jsdoc comment affects a function signature (playground link), and non-jsdoc comments are ignored (playground link).
nx-cloudbot commentedFeb 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commitec9a1ea.
☁️Nx Cloud last updated this comment at |
unified-signatures] support ignoring overload signatures with different JSDoc commentsunified-signatures] support ignoring overload signatures with different JSDoc commentscodecovbot commentedFeb 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #10781 +/- ##==========================================+ Coverage 87.27% 87.29% +0.01%========================================== Files 453 453 Lines 15833 15840 +7 Branches 4639 4643 +4 ==========================================+ Hits 13819 13827 +8 Misses 1658 1658+ Partials 356 355 -1
Flags with carried forward coverage won't be shown.Click here to find out more.
|
LukeAbby commentedFeb 6, 2025
Not a maintainer so just a random guy's two cents. Do you think it'd be worth considering a sibling option to NOT suggest unifying two overloads when one is deprecated and the other isn't? This is actually what the original issue's code example was. The issue I see with doing it based off of any JSDoc change is that if you dutifully document the optional parameter (but leave the rest of the JSDoc identical) then it'll be missed. |
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.
LGTM, another very thorough investigation 🔥. Just requesting changes on simplifying the block-comment/JSDoc checking.
| return false; | ||
| } | ||
| const lines = comment.value.split(LINEBREAK_MATCHER); |
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.
[Feature] I tried replacing the rest of this function withreturn true and only two tests failed:
/** * This signature does something. **/
/* * This signature does something. */
ACK that these are technically not correctly formed for JSDocs, but TypeScript in VS Code still shows them well:
Proposal: let's just go off of whether it's aBlock?
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.
Oh, nice catch!
I could make TypeScript in VSCode to show this in the first example, but I couldn't do it in the second one. Reverse engineering this a bit, it seems the comment has to start with at least two `*'s (playground link):
// seem to show when the function is hovered/** * This signature does something. */declarefunctionf(x:number):unknown;//doesn't seem to show when the function is hovered/* * This signature does something. */declarefunctiong(x:number):unknown;
I agree though, that matching TypeScript's behavior would probably be pretty complex (and possibly not worth the extra complexity), especially since invalid jsdoc also applies.
Also, for comments that aren't recognized by TypeScript (like the second example), I think it may be better not to report them since it's most likely a typo on the developer's part (or at least I assume so).
I've updated my PR to match your suggestion, thanks! 🚀
Agreed, this seems like a separate bug to me. |
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.
02d9d73 intotypescript-eslint:mainUh oh!
There was an error while loading.Please reload this page.
| datasource | package | from | to || ---------- | -------------------------------- | ------ | ------ || npm | @typescript-eslint/eslint-plugin | 8.24.0 | 8.26.0 || npm | @typescript-eslint/parser | 8.24.0 | 8.26.0 |## [v8.26.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8260-2025-03-03)##### 🚀 Features- **eslint-plugin:** \[unified-signatures] support ignoring overload signatures with different JSDoc comments ([#10781](typescript-eslint/typescript-eslint#10781))- **eslint-plugin:** \[explicit-module-boundary-types] add an option to ignore overload implementations ([#10889](typescript-eslint/typescript-eslint#10889))- **eslint-plugin:** \[no-unused-var] handle implicit exports in declaration files ([#10714](typescript-eslint/typescript-eslint#10714))- support TypeScript 5.8 ([#10903](typescript-eslint/typescript-eslint#10903))- **eslint-plugin:** \[no-unnecessary-type-parameters] special case tuples and parameter location arrays as single-use ([#9536](typescript-eslint/typescript-eslint#9536))##### 🩹 Fixes- **eslint-plugin:** \[no-unnecessary-type-assertion] handle unknown ([#10875](typescript-eslint/typescript-eslint#10875))- **eslint-plugin:** \[no-invalid-void-type] report `accessor` properties with an invalid `void` type ([#10864](typescript-eslint/typescript-eslint#10864))- **eslint-plugin:** \[unified-signatures] does not differentiate truly private methods ([#10806](typescript-eslint/typescript-eslint#10806))##### ❤️ Thank You- Andrea Simone Costa [@jfet97](https://github.com/jfet97)- Dirk Luijk [@dirkluijk](https://github.com/dirkluijk)- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)- Yukihiro Hasegawa [@y-hsgw](https://github.com/y-hsgw)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.## [v8.25.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8250-2025-02-24)##### 🚀 Features- **eslint-plugin:** \[no-misused-spread] add suggestions ([#10719](typescript-eslint/typescript-eslint#10719))##### 🩹 Fixes- **eslint-plugin:** \[prefer-nullish-coalescing] report on chain expressions in a ternary ([#10708](typescript-eslint/typescript-eslint#10708))- **eslint-plugin:** \[no-deprecated] report usage of deprecated private identifiers ([#10844](typescript-eslint/typescript-eslint#10844))- **eslint-plugin:** \[unified-signatures] handle getter-setter ([#10818](typescript-eslint/typescript-eslint#10818))##### ❤️ Thank You- Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal)- Ronen Amiel- YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.## [v8.24.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8241-2025-02-17)##### 🩹 Fixes- **eslint-plugin:** \[class-methods-use-this] check `accessor` methods with a function initializer ([#10796](typescript-eslint/typescript-eslint#10796))- **eslint-plugin:** \[no-misused-promises] don't report on `static` `accessor` properties ([#10814](typescript-eslint/typescript-eslint#10814))- **eslint-plugin:** \[no-deprecated] don't report on deprecated `accessor` property declaration ([#10813](typescript-eslint/typescript-eslint#10813))- **eslint-plugin:** \[explicit-member-accessibility] check `accessor` class properties for missing accessibility modifier ([#10805](typescript-eslint/typescript-eslint#10805))- **eslint-plugin:** \[explicit-module-boundary-types] check `accessor` class properties with a function initializer ([#10804](typescript-eslint/typescript-eslint#10804))- **eslint-plugin:** \[prefer-return-this-type] check `accessor` properties with a function initializer ([#10794](typescript-eslint/typescript-eslint#10794))- **eslint-plugin:** \[consistent-generic-constructors] check `accessor` class properties ([#10789](typescript-eslint/typescript-eslint#10789))- **eslint-plugin:** \[no-unsafe-assignment] report on an `any` value assigned as an initializer of an `accessor` property ([#10785](typescript-eslint/typescript-eslint#10785))- **eslint-plugin:** \[no-unnecessary-template-expression] ignore enum and enum members ([#10782](typescript-eslint/typescript-eslint#10782))- **eslint-plugin:** \[no-inferrable-types] handle accessor ([#10780](typescript-eslint/typescript-eslint#10780))##### ❤️ Thank You- Ronen Amiel- YeonJuanYou can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
This PR attempts to tackle#10520 and adds the
ignoreOverloadsWithDifferentJSDocoption. When enabled, the following is considered valid:Some thoughts/notes:
I've only considered JSDoc comments that annotate a function signature, not ones that annotate its params (or return type annotation, see below), so the following is reported, even though it has similar issues with unifying two signatures with different jsdoc comments:
This is tricky, as the following overloads, for example, can be unified (and should be reported):
I'm a bit unsure if this should be checked or not, as it wasn't described on the issue. I'd love to hear opinions on this, and would be happy to add a check for this (although this seems somewhat complex 🙈).
I didn't add any checks to an annotated return type, as return type annotations are checkedby comparing the source code text.
So the following signatures already count as unique (playground link):
Unrelated to the change in the PR: Considering the paragraph above, it does have some false positives, as the following should be reported but doesn't:
This also includes random, non-jsdoc comments, spaces, line-breaks, etc (playground link). Should I open a separate issue on this?
I think it can at least be compared with something similar to
isNodeEqualso spacing/line-breaks don't affect it.