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

chore(docs): add ability to generate xmldocs #5164

Merged
merged 3 commits into from Feb 15, 2021
Merged

chore(docs): add ability to generate xmldocs #5164

merged 3 commits into from Feb 15, 2021

Conversation

avodovnik
Copy link
Contributor

This introduces a naive XmlDocs renderer for Markdown nodes. It needs to be called separately for now, i.e. the generateSourceCodeComment in Documentation doesn't integrate with it yet, but I'll add that somewhere down the line.

lastNode = innerRenderXmlNode(node, lastNode, summary, examples, maxColumns);
});

if (summary.length == 1) { // just the <summary> node
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to handle this edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's supposed to be a cheap way of getting rid of:

    /// <summary>
    /// </summary>

but instead of ignoring it, I might be better off outputting a warning as that means it's not documented.

}

// we might have a stray list, if the <li> is the last element
if (lastNode && lastNode.type === 'li') {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like md's list items differ from the xmldoc ones in a way that it wants a wrapping container. I would render lists as md, i.e. via prepending - . That'll erase this lastNode handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, VS isn't smart enough to actually render that in a meaningful way. So I'll always need to wrap it in a <list> if I want it to display at least somewhat nicely to the end user.

This:

    /// <summary>
    /// Remember, that sometimes, you need to do the following:
    /// - Test 1
    /// - Test 2
    /// - Test 3
    /// - Test 4
    /// 
    /// But there are times, when you want to do
    /// <list type="bullet">
    /// <item><description>Something smart</description></item>
    /// <item><description>Something smarter</description></item>
    /// <item><description>The really smart thing</description></item>
    /// <item><description>Just a list</description></item>
    /// </list>
    /// Tis a thing <seealso 
    /// cref="Browser"/> test.
    /// <a href="https://www.google.com">This is a link.</a>
    /// </summary>

turns into this:

image

* @param {number} maxColumns
*/
function renderXmlDoc(nodes, maxColumns) {
const summary = [];
Copy link
Member

Choose a reason for hiding this comment

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

Give us some 2 space indents!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now... :-) Looks like my settings were wrong. Fixed that as well.


if (node.type === 'text') {
// clear up the list, if there was one
if (lastNode && lastNode.type === 'li') {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't wrap single-liners with {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've mostly avoided doing that now, but then eslint seems to have thrown a fit. Either way, point taken, thanks!

}

// if the previous node was not li, start list
if (lastNode && lastNode.type !== 'li') {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely don't use , your summary would actually read better without 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.

I've reworked that code, so it's a bit more digestable, but I decided to keep using the li part for the reasons outlined above.


// TODO: this should really move to the examples array
if (node.type == 'code' && node.codeLang == "csharp") {
summary.push('<![CDATA[');
Copy link
Member

Choose a reason for hiding this comment

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

Sneaky! i like it!

a.push('');
};

let escapedText = node.text;
Copy link
Member

Choose a reason for hiding this comment

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

probably const escapedText = escape(node.text) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't ran into text I needed to escape, so I've omitted the escaping part for now.

utils/doclint/xmlDocumentation.js Outdated Show resolved Hide resolved
Copy link
Contributor

@JoelEinbinder JoelEinbinder left a comment

Choose a reason for hiding this comment

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

Code looks fine. I'd have some style nits, but I think they are all because this file is not covered by eslint. We should probably restore eslint for .js files or convert doclint to typescript.

@avodovnik
Copy link
Contributor Author

@JoelEinbinder I reformatted using 2 space tabs, instead of 4, and I ran eslint hopefully fixing the majority of the nits. Hopefully this is approvable now, and I can improve it as I go along :-)

@avodovnik avodovnik merged commit 9dd443e into microsoft:master Feb 15, 2021
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.

None yet

3 participants