-
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] Allow customization of table of contents #1265
Conversation
Could we replace the api-documenter-oufr test with a generic project that isolates specific interesting test cases? Some possible concerns:
Also, the exercise of creating a different test may help us to think about other possible applications of your feature beyond OUIFR's requirements. |
* that are matched with the filters provided. Everything else will be placed under a catchAll category | ||
* that is highly recommended to be provided. | ||
*/ | ||
tocConfig: IYamlTocFile; |
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 feel like there should be a separation between the toc.yml file format (as specified by DocFX) versus the api-documenter.json file format (which is specific to API Documenter, and may eventually be usable with non-DocFX outputs).
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.
How to ensure then that the TOC map user provides is somewhat aligned with DocFX specification for a TOC?
@octogonz I removed the test I added initially and modified the existing one by adding some inline custom tags. I've also adjusted the debugger to run it and it generates the expected TOC. |
@@ -490,7 +529,8 @@ export class YamlDocumenter { | |||
JsonFile.validateNoUndefinedMembers(dataObject); | |||
|
|||
let stringified: string = yaml.safeDump(dataObject, { | |||
lineWidth: 120 | |||
lineWidth: 120, | |||
noRefs: this._config && this._config.noDuplicateEntries ? true : false |
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.
Why is noRefs
enabled here? Generally we've shied away from this YAML feature because it introduces reference nodes that interfere with diffs and don't seem to serve any useful purpose. (A properly defined file format normally doesn't include a significant amount of redundant data.) As far as I know the deserialized result is unaffected by this feature, so it's purely cosmetic.
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 can be safely removed I believe as it's more of a case-specific thing I tried. In our case, we are loading 4 api.json files: OUFR, styling, utilities, and merge-styles. OUFR on its hand was also re-exporting 2 of these: styling and utilities, so I ended up having lots of duplicates in the navigation. But we will handle this differently now so no need for it now. Forgot to remove it 😄
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 reference nodes that you say about are actually introduced if this feature is not toggled on.
…ponding DocumenterConfig class
…emitted by an "api-documenter init" action; for now we'll use this as a sample to track the intended config file contents
@iclanton I would appreciate having this experimental addition merged and a release triggered so I can start testing it. Thanks! |
apps/api-documenter/src/documenters/ExperimentYamlDocumenter.ts
Outdated
Show resolved
Hide resolved
apps/api-documenter/src/documenters/ExperimentYamlDocumenter.ts
Outdated
Show resolved
Hide resolved
apps/api-documenter/src/documenters/ExperimentYamlDocumenter.ts
Outdated
Show resolved
Hide resolved
@@ -1,6 +1,8 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. | |||
// See LICENSE in the project root for license information. | |||
|
|||
// tslint:disable:member-ordering | |||
|
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.
@octogonz - did you ever update this rule?
common/changes/@microsoft/api-documenter/FabricDocumenter_2019-05-06-23-48.json
Outdated
Show resolved
Hide resolved
common/changes/@microsoft/api-documenter/FabricDocumenter_2019-05-06-23-48.json
Outdated
Show resolved
Hide resolved
@@ -6,6 +6,7 @@ | |||
|
|||
Example base class | |||
|
|||
|
|||
<b>Signature:</b> |
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 adding this blank line intentional? Seems like it shouldn't be.
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.
Removed this blank line although it was autogenerated when I ran the test. So with future runs it might get added again.
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.
That makes it seem like a regression was introduced.
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.
That would be highly unlikely because we did not touch the MarkdownDocumenter
in this PR. I added a docCategory
tag to the test files that might have somehow influenced the output.
@@ -26,7 +26,7 @@ if (process.argv.indexOf('--production') >= 0) { | |||
|
|||
// Run the API Documenter command-line | |||
executeCommand('node node_modules/@microsoft/api-documenter/lib/start ' | |||
+ 'yaml --input-folder etc --output-folder etc/yaml'); | |||
+ 'generate --input-folder etc --output-folder etc/yaml'); |
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.
generate [](start = 5, length = 8)
Should this be an entirely separate test?
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 new generate
action is meant to replace the yaml
action. It's still under experimental title but as we polish it the plan is having just one action generate
that will look for an api-documenter.json
config file where we would specify what documenter to be used: yaml vs markdown.
export class GenerateAction extends BaseAction { | ||
constructor(parser: ApiDocumenterCommandLine) { | ||
super({ | ||
actionName: 'generate', |
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.
generate [](start = 19, length = 8)
This seems like a weird verb. Maybe "experimental-yaml" or something?
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.
See this comment. We discussed it with @octogonz that we might wanna move to a using this generic generate
action along with a config file that will command which documenter to be used.
@@ -17,7 +17,7 @@ | |||
{ | |||
"kind": "Class", | |||
"canonicalReference": "(DocBaseClass:class)", | |||
"docComment": "/**\n * Example base class\n *\n * @public\n */\n", | |||
"docComment": "/**\n * Example base class\n *\n * {@docCategory DocBaseClass}\n *\n * @public\n */\n", |
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.
@iclanton I bet this change to the documentation is causing the addition of a blank line. MarkdownDocumenter
does not know how to handle inline custom tags and it skips {@docCategory DocBaseClass}
and is left with \n
right after. So this is specific to only this case and this test project. I don't consider it as a regression. Perhaps a MarkdownDocumenter
bug that needs to be fixed in a separate PR.
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.
Cool.
@@ -17,7 +17,7 @@ | |||
{ | |||
"kind": "Class", | |||
"canonicalReference": "(DocBaseClass:class)", | |||
"docComment": "/**\n * Example base class\n *\n * @public\n */\n", | |||
"docComment": "/**\n * Example base class\n *\n * {@docCategory DocBaseClass}\n *\n * @public\n */\n", |
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.
Cool.
@octogonz here is a draft of the changes that generates a custom TOC. I tried to make them as generic as possible so that not to break anything for the existing behavior. Also, I've put some comments on some missing types that we need to define. I've verified that without a config file provided it still generates what it was before.
Let me know how else I can help!
cc: @natalieethell , @aditima , @KevinTCoughlin