-
Notifications
You must be signed in to change notification settings - Fork 604
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-documenter] Add logic to disambiguate duplicate UID, file, or toc entries #1331
Conversation
Note that this has a minor conflict with #1326 that would need to be resolved in either branch if merged. |
- api-documenter-test.DocClass1.exampleFunction_1 | ||
- api-documenter-test.DocClass1.interestingEdgeCases | ||
- api-documenter-test.DocClass1.deprecatedExample() | ||
- 'api-documenter-test.DocClass1.exampleFunction(string,string)' |
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.
@rbuckton What would you do with this input? 🙃
export declare function pathologicalExample(options: {
args: {
[name: string]: string | boolean;
};
buildErrorIconPath?: string;
buildSuccessIconPath?: string;
distFolder: string;
gulp: GulpProxy | Gulp;
isRedundantBuild?: boolean;
jestEnabled?: boolean;
libAMDFolder?: string;
libES6Folder?: string;
libESNextFolder?: string;
libFolder: string;
onTaskEnd?: (taskName: string, duration: number[], error?: any) => void;
onTaskStart?: (taskName: string) => void;
packageFolder: string;
production: boolean;
properties?: {
[key: string]: any;
};
relogIssues?: boolean;
rootPath: string;
shouldWarningsFailBuild: boolean;
showToast?: boolean;
srcFolder: string;
tempFolder: string;
uniqueTasks?: IExecutable[];
verbose: boolean;
} & Partial<ICopyConfig>): void;
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.
items:
- uid: package.pathologicalExample
name: package.pathologicalExample
type: function
syntax:
parameters:
- id: options
type:
- anonymous0:local
return:
type:
- void
references:
- uid: anonymous0:local
name: |> <content of options type>
spec.typeScript:
- name: '{\n args: {\n [name: string]: string | boolean;\n }; <snip> gulp: '
- name: GulpProxy
uid: otherpackage.GulpProxy
- name: ' | '
- name: Gulp
uid: gulp.Gulp
- name: ';\n isRedundantBuild?: <snip> uniqueTasks?: '
- name: IExecutable
uid: otherpackage.IExecutable
- name: '[];\n verbose: boolean;\n} & '
- name: Partial
uid: Partial
- name: '<'
- name: ICopyConfig
uid: otherpackage.ICopyConfig
- name: '>'
- uid: otherpackage.GulpProxy
name: GulpProxy
- uid: gulp.Gulp
name: Gulp
- uid: otherpackage.IExecutable
name: IExecutable
- uid: otherpackage.ICopyConfig
name: ICopyConfig
You also could have a uid of {{args:{{[name:string]~string|boolean}},buildErrorIconPath?:string,buildSuccessIconPath?:string,...}&Partial{ICopyConfig}
(with the ...
replaced) since it doesn't need to be human readable, just machine readable.
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.
The entry in references
is effectively the same as an Excerpt
, with some additional information (uid
, nameWithType
, fullName
).
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 doesn't need to be human readable, just machine readable.
@rbuckton From a technical perspective, this may be true. But I'm pretty uncomfortable about designing a system that emits huge unwieldy expressions in a file that people may have to understand/debug.
Also, what about the other related ID problems called out in #1308? We wouldn't want these huge expressions to appear in:
- Markdown URLs
- DocFX URLs
- Titles that appear in member tables in the docs
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.
Complex types should probably have aliased UIDs (possibly based on a hash of the stable UID). They would point to an entry in the yaml references
list which would have the expanded text/excerpt that breaks down the type into hyperlinkable tokens.
I plan to abandon this PR as I am truly starting to believe that Uid generation is better handled by |
Here's the new approach I'm thinking about: https://github.com/rbuckton/web-build-tools/pull/1/files#diff-5bac56083efa60bf63727db8afb690e3 The It does generate ugly UIDs for complex types currently. I'm considering a mechanism for replacing what would be a very complex UID with a simpler reference UID like The upside of using a stable UID generation mechanism that is driven by the TS TypeChecker, is that two different projects that depend on the same third project will have the same UIDs generated for their references, so that if the documentation for all three is merged the referenced links will work appropriately. |
See https://gist.github.com/rbuckton/dcd5cf51b0a4d0fc7fe64405c1ec4edf for some additional thoughts I have. Instead of items:
- uid: package.pathologicalExample(@0)
name: package.pathologicalExample(@0)
type: function
syntax:
parameters:
- id: options
type:
- package.pathologicalExample@0
return:
type:
- void
references:
- uid: package.pathologicalExample@0
name: |> <content of options type>
spec.typeScript:
- name: '{\n args: {\n [name: string]: string | boolean;\n }; <snip> gulp: '
- name: GulpProxy
uid: otherpackage.GulpProxy
- name: ' | '
- name: Gulp
uid: gulp.Gulp
- name: ';\n isRedundantBuild?: <snip> uniqueTasks?: '
- name: IExecutable
uid: otherpackage.IExecutable
- name: '[];\n verbose: boolean;\n} & '
- name: Partial
uid: Partial
- name: '<'
- name: ICopyConfig
uid: otherpackage.ICopyConfig
- name: '>'
- uid: otherpackage.GulpProxy
name: GulpProxy
- uid: gulp.Gulp
name: Gulp
- uid: otherpackage.IExecutable
name: IExecutable
- uid: otherpackage.ICopyConfig
name: ICopyConfig |
I need to revisit this, as #1337 only dealt with the uid generation part, I'm still concerned about diambiguating TOC items for cases like: export interface Foo {}
export declare function Foo(): Foo; Where the TOC might be generated as:
Its not so much an issue now as it will be if/when we fix how namespaces are added to the YAML (which also requires a DocFX fix). |
Closing in favor of #1536 |
This PR adds logic to disambiguate duplicate UIDs, files, or toc entries.
This may require further iterations to come up with the best approach for synthesizing stable UIDs, however it currently uses the following rules:
UID/Path
package.Namespace.ItemName
)m(x: number)
the UID would be<parent>.m(number)
m<T>(x: T, number)
the UID would be<parent>.m``1(``0,number)
:<item kind>
is appended if the item.<package part>/<non-package part><disambiguation>.yml
_
._<n>
suffix is added wheren
is incremented until the UID is no longer ambiguous.TOC Items
TOC items are disambiguated after creation by following the
uid
of the toc item back to its original item.uid
are not disambiguated.C (Class)
,C (Interface)
).(<n>)
suffix is added wheren
is incremented until the TOC item is no longer ambiguous.Fixes #1308