-
Notifications
You must be signed in to change notification settings - Fork 603
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
[api-extractor] Allow separate release tags for overloaded functions and methods #1544
[api-extractor] Allow separate release tags for overloaded functions and methods #1544
Conversation
…r different signatures
…into danade/FunctionAndMethodAccessability
@D4N14L the TSDoc tags such as |
@D4N14L Thanks for implementing this BTW! It's a pretty useful feature and we've had many requests for it. |
build-tests/api-extractor-scenarios/etc/test-outputs/functionOverload/public-rollup.d.ts
Show resolved
Hide resolved
…into danade/FunctionAndMethodAccessability
@@ -110,15 +110,25 @@ export class DtsRollupGenerator { | |||
} |
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.
Is this check still needed? It seems redundant since we're checking each individual declaration below.
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.
I thought about this when adding... I figured keeping the old behaviour was alright (rolls up the comments to one symbol when they're all excluded) however, it makes more sense to have it consistent. I'll make the change
Overall this PR looks fine, however there were a couple places where the logic was a little difficult to follow, or did not consider possible configurations where the validation rules are suppressed. Here's a PR with a proposed simplification: D4N14L#1 I'll add some comments to that PR explaining the issues. |
…bility Proposed changes
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 good. If you don't have any further changes or validations you'd like to do, then I'm good with merging. Thanks again for implementing this!
@octogonz I was still going to look at removing that duplicate check above, however I'm noticing that it actually breaks the build on api-extractor-test-04 (it tries to export Lib1Interface even though it's only available in @Alpha. If you'd like to take a look at this feel free, but if not I was going to look into it on Monday |
@D4N14L The reason is that the So this your existing implementation seems like the correct behavior to me: If all declarations are trimmed, then we shouldn't export the symbol, and we can put one consolidated comment. Whereas if at least one declaration is untrimmed, then we need to export the symbol, and should put individual comments. To make this distinction more explicit, I'm going to change the text of the emitted comment slightly for the second case. But otherwise I think we're good to merge this PR. 👍 |
…e", whereas when we trim a declaration it says "Excluded declaration from this release type"
This feature was published with API Extractor 7.5.0 |
…dAccessability [api-extractor] Allow separate release tags for overloaded functions and methods
(PR description added by @octogonz since it was missing)
I assume this PR is related to #972 (comment):