-
Notifications
You must be signed in to change notification settings - Fork 536
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(api-markdown-documenter): Add and use a ValidApiItemKind
type alias which excludes the None
value
#23384
refactor(api-markdown-documenter): Add and use a ValidApiItemKind
type alias which excludes the None
value
#23384
Conversation
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.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- tools/api-markdown-documenter/src/api-item-transforms/ApiItemTransformUtilities.ts: Evaluated as low risk
- tools/api-markdown-documenter/src/api-item-transforms/TransformApiItem.ts: Evaluated as low risk
- tools/api-markdown-documenter/src/api-item-transforms/configuration/DocumentationSuiteOptions.ts: Evaluated as low risk
- tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiClass.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
tools/api-markdown-documenter/src/utilities/ApiItemUtilities.ts:58
- The error message should include more context about where the error occurred. Consider updating it to:
Encountered an API item with kind "None" in getApiItemKind function: "${apiItem.displayName}".
throw new Error(`Encountered an API item with kind "None": "${apiItem.displayName}".`);
@@ -112,7 +112,7 @@ declare namespace ApiItemUtilities { | |||
export { ApiItemUtilities } | |||
|
|||
// @public | |||
export type ApiMemberKind = Omit<ApiItemKind, ApiItemKind.EntryPoint | ApiItemKind.Model | ApiItemKind.None | ApiItemKind.Package>; |
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 was a bug. Should have been Exclude
and not Omit
.
hierarchyBoundaries: HierarchyBoundaries, | ||
): boolean { | ||
if (kind === ApiItemKind.Model) { |
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.
Handling was not exhaustive, revealed by fixing ApiMemberKind
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
*/ | ||
export function getApiItemKind(apiItem: ApiItem): ValidApiItemKind { | ||
switch (apiItem.kind) { | ||
case ApiItemKind.None: { |
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.
Curious: what would have an item kind of None
?
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.
For our purposes, nothing, and would signal that we encountered corrupted input or something. My guess as to why the value exists at all is for use in the various mixin "ApiItem" types they export - they aren't real API item types, so they would be tagged as None
.
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.
No, I take that back. They don't use a "kind" at all. Searching Rushstack's repo, I see no hits for ApiItemKind.None
at all... So I actually have no idea why that type exists. Something for me to ask Pete about at some point.
No valid
ApiItem
will have kind "None". There are a number of places where we are forced to assert that an item's kind is not "None" to make checks exhaustive, and there are other places where we probably should have been.This PR adds a helper type alias and a helper function to encapsulate the validity checking.