Skip to content
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

Merged
merged 21 commits into from
May 16, 2019

Conversation

Vitalius1
Copy link
Contributor

@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

@Vitalius1 Vitalius1 changed the title Fabric documenter [api-documenter] - config file for custom TOCs May 6, 2019
@Vitalius1 Vitalius1 marked this pull request as ready for review May 6, 2019 23:40
@octogonz octogonz changed the title [api-documenter] - config file for custom TOCs [api-documenter] Allow customization of table of contents May 7, 2019
@octogonz
Copy link
Collaborator

octogonz commented May 7, 2019

Could we replace the api-documenter-oufr test with a generic project that isolates specific interesting test cases? Some possible concerns:

  • These files are somewhat large (e.g. office-ui-fabric-react.api.json is 5MB). This increases the git clone footprint, and GitHub's UI generally chokes up on large files.

  • The content contains lots of keywords that are unrelated to our projects, and may reduce the quality of search results (e.g. GitHub search).

  • Although your usage here is okay, in general we're very careful about copying content between GitHub projects to avoid copyright issues. Even if both projects are owned by Microsoft, the third-party notice requirements may be different.

Also, the exercise of creating a different test may help us to think about other possible applications of your feature beyond OUIFR's requirements.

@octogonz octogonz force-pushed the FabricDocumenter branch from 105ceef to 303c49d Compare May 7, 2019 01:03
* 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;
Copy link
Collaborator

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).

Copy link
Contributor Author

@Vitalius1 Vitalius1 May 7, 2019

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?

@Vitalius1
Copy link
Contributor Author

Vitalius1 commented May 7, 2019

@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.
The existing test already had lots of different API items to test so we can just grow that generic example project with more edge cases as we find to test.

@@ -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
Copy link
Collaborator

@octogonz octogonz May 7, 2019

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.

Copy link
Contributor Author

@Vitalius1 Vitalius1 May 7, 2019

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 😄

Copy link
Contributor Author

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.

@octogonz octogonz force-pushed the FabricDocumenter branch from d77aee0 to 34339cd Compare May 7, 2019 05:15
@Vitalius1
Copy link
Contributor Author

@iclanton I would appreciate having this experimental addition merged and a release triggered so I can start testing it. Thanks!

@@ -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

Copy link
Member

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?

@@ -6,6 +6,7 @@

Example base class


<b>Signature:</b>
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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');
Copy link
Member

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?

Copy link
Contributor Author

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',
Copy link
Member

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?

Copy link
Contributor Author

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",
Copy link
Contributor Author

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.

Copy link
Member

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants