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

Can the format of the auto-generated doc comments be updated? #1012

Closed
baronfel opened this issue Feb 7, 2019 · 30 comments
Closed

Can the format of the auto-generated doc comments be updated? #1012

baronfel opened this issue Feb 7, 2019 · 30 comments
Labels

Comments

@baronfel
Copy link
Contributor

baronfel commented Feb 7, 2019

I noticed while working on doc generation in MiniScaffold that the markdown rendered by FSharp.Formatting is a little rough when the sections are delimited by * instead of # headings. When # are provided the parser gives a lot more detail to the structure that we can use to render sections a bit better, so I'd like to see if you are open to changing the current auto-generated comment headings.

Let me give an example of the structure we get back from FSharp.Formatting in different scenarios. Here is our sample code:

module Say =

    /// **Person**
    type Person = {
        /// First
        First : string
        Last : string
        FavoriteNumber : int
        DateOfBirth : DateTimeOffset
    }


    /// **Description**
    ///
    /// **Parameters**
    ///   * `person` - parameter of type `Person`
    ///
    /// **Output Type**
    ///   * `string`
    ///
    /// **Exceptions**
    ///
    let helloPerson (person : Person) =
        sprintf
            "Hello %s %s. You were born on %s and your favorite number is %d."
            person.First
            person.Last
            (person.DateOfBirth.ToString("o"))
            person.FavoriteNumber

    /// #### Description
    /// I do nothing, ever.
    ///
    /// #### Parameters
    ///   * `name` - parameter of type `'a`
    ///
    /// #### Output Type
    ///   * `unit`
    ///
    /// #### Exceptions
    /// None
    let nothing name =
        name |> ignore


    /// **Description**
    ///
    /// **Parameters**
    ///   * `name` - parameter of type `string`
    ///
    /// **Output Type**
    ///   * `string`
    ///
    /// **Exceptions**
    ///
    [<CompiledName("Hiya")>]
    let hello name =
        sprintf "Hello %s" name

and here is the matching Comment structure for the helloPerson comment block:

{Blurb =
  "<p><strong>Description</strong></p>
<p><strong>Parameters</strong>
* <code>person</code> - parameter of type <a href="mylib-_1-say-person.html" title="Person"><code>Person</code></a></p>
<p><strong>Output Type</strong>
* <code>string</code></p>
<p><strong>Exceptions</strong></p>

";
 FullText =
  "<p><strong>Description</strong></p>
<p><strong>Parameters</strong>
* <code>person</code> - parameter of type <a href="mylib-_1-say-person.html" title="Person"><code>Person</code></a></p>
<p><strong>Output Type</strong>
* <code>string</code></p>
<p><strong>Exceptions</strong></p>

";
 Sections =
  [[<default>, <p><strong>Description</strong></p>
<p><strong>Parameters</strong>
* <code>person</code> - parameter of type <a href="mylib-_1-say-person.html" title="Person"><code>Person</code></a></p>
<p><strong>Output Type</strong>
* <code>string</code></p>
<p><strong>Exceptions</strong></p>
]];}

Some things to note here are

  • The comments are all in one 'section' (), since there are no headings to delimit them.
  • There's no semantic structure between the comments. If you spliced FullText in somewhere you'd get a messy, inline blob:
    image
  • this looks OK in ionide due to special handling

Now let's take a look at what the structure for the nothing function is:

{Blurb = "
";
 FullText =
  "<h2>Description</h2>
<p>I do nothing, ever.</p>
<h2>Parameters</h2>
<ul>
<li><code>name</code> - parameter of type <code>'a</code></li>
</ul>
<h2>Output Type</h2>
<ul>
<li><code>unit</code></li>
</ul>
<h2>Exceptions</h2>
<p>None</p>

";
 Sections =
  [[<default>, 
]; [Description, <p>I do nothing, ever.</p>

];
   [Parameters, <ul>
<li><code>name</code> - parameter of type <code>'a</code></li>
</ul>

];
   [Output Type, <ul>
<li><code>unit</code></li>
</ul>

];
   [Exceptions, <p>None</p>
]];}

In this case the FullText is useful, semantic markup and renders nicely by default. However, the bigger win here is that the hnodes are parsed into sections and so can be treated differently based on the kind of section they are. We could begin to establish some convention around the content of the particular sections, for example.

Also, with four # this still looks good in ionide out of the box:
image

Having those sections makes it really easy to write code consuming the comment, like I did here: https://github.com/TheAngryByrd/MiniScaffold/blob/28dc42dd88018eddfa1a35bea99618787d0fc6ed/Content/Library/docsSrc/templates/partMembers.fsx#L99-L114. Right now the section name is just an h2, but it could be any kind of styling I wanted.

What do you think?

@MangelMaxime
Copy link
Contributor

Personnally, I don't this proposition for several reasons.

#### means h4 and should be generated in your html page only if you have an h3 before. Which should have an h2 before, and so on.

I guess you put #### here because you wanted a similar look inside of Ionide to what ** ** ouput.

But nothing garantee, that h4 will render nicely in term of size for every CSS settings.

So here, we are making several assumption.

  1. Everyone will use F# formatting to generate their docs and it will generate correctly structure/organized HTML
  2. The size used for h4 looks nice in term of size compare to standard text in the CSS use by the person
  3. They don't use plugins to generate anchor for their heading elements.

If we use #### to mark the section then highlights in the markdown doesn't work nicely. The titltes are highlighted in white because they are considered as invalid markup by the markdown grammar.

They should be in red if the grammar accepted them as valid:

capture d ecran 2019-02-07 a 17 43 38

capture d ecran 2019-02-07 a 17 43 53


It's also important to note that the markdown parser used by F# Formatting isn't 100% compliant with the markdown specs and so it's possible that the wrong display where it display * after Parameters or Output type comes from this.

For me, we should not change the current generation. But a fix should be provided to F# Formatting or just use a pre-processor steps in your build process to convert the comments into "optimized" comment for F# Formatting. Because current, format is easy enough to process using one regex to archieve the format you are expecting.

@baronfel
Copy link
Contributor Author

baronfel commented Feb 7, 2019

That's fine, I'd be happy with a single # as well to delineate proper sections. The FSharp.Formatting parser just treats any number # as a new section from what I can tell so there's no semantic difference there. Personally I think that just emitting the RawText provided by FSharp.Formatting is a code-smell, so doing what we do in miniscaffold where we iterate the sections is a more semantic and style-able pattern.

It's also important to note that the markdown parser used by F# Formatting isn't 100% compliant with the markdown specs and so it's possible that the wrong display where it display * after Parameters or Output type comes from this.

You're right here, I missed that FSF isn't converting the * to a ul with li elements. That's worth fixing too. It's interesting to note that once the # are introduced the individual sections do render correctly as lists with items. But I think that most people would expect to use headings to split sections in markdown, don't you agree? That's really what I'm asking for here.

@Krzysztof-Cieslak
Copy link
Member

That's fine, I'd be happy with a single # as well to delineate proper sections.

I'm pretty sure this looks bad in VSCode tooltips tho. And looking nice in VSCode tooltips is a strong requirement for this feature - otherwise, people will just hate it. I'd rather modify/fix F#.Formatting - as this is something we can control (unlike VSCode tooltips rendering).

@MangelMaxime
Copy link
Contributor

@baronfel If we use a single # then it became an h1 and you should have only one h1 per page in theory.

And in VSCode you get this result:

capture d ecran 2019-02-07 a 18 29 27

For me, the current way of writing docs comments is just syntax sugar to replace the XML version.
So they are kind of a sub markup. And so just as the XML version it should be parsed and formatted.

Also, each docs comments should be considered independently because they are not related to each other. So each time we create a docs comment, we are in fact creating a "section".

If F# formatting is considering that it can parse .fs files directly, it should be its job to know that a doc comment creates a section. If we were working from the XML files generated by dotnet, we would probably consider each summary as a section and either parse the XML structure or the "sub-markdown markup" in order to generate the expected output.

And using # doesn't fix the syntax highlight problem :)

I am not telling that the correct way of doing is perfect. But I see more cons to change the docs comments when the bug is inside F# formatting.

@TheAngryByrd
Copy link
Member

Looking at Rust's doc comments they use #/h1 tags for their sections. They use it to generate sections for their html documentation, even though they have "many" #/h1 per page.

They do however look gross in VSCode too.

screen shot 2019-02-24 at 10 07 20 am

I'm not sure this is really a bug in FSharp.Formatting. Overloading strong tags to be sections feels wrong semantically as well. I do think at some point we're going to be breaking semantics of some intended use, which means maybe markdown is the wrong thing to be using in general.

What about if we used #/h1 tags for documentation generation, then when VSCode need to display these, FSAC maps those headers to something that display nicer like the strong tag?

@Krzysztof-Cieslak
Copy link
Member

Do we have any consensus regarding this issue? TBF, I'm in favour of just leaving it as-is - it seems we don't have any solution that makes the situation better for everyone.

@Krzysztof-Cieslak
Copy link
Member

I remember talking with someone (Don? Eugene? ) about it on last F# eXchange - suggestion was to actually generate XMLDoc format. We can interpret XMLDoc format correctly (i.e. it will produce nicely formatted tooltip in Code) and it will be more compatible with the other tooling (which often doesn't render markdown - VS/Rider)

@baronfel
Copy link
Contributor Author

baronfel commented Jun 24, 2019

I've done some experimentation here and the xmldoc parsing could be better in general. In markdown we'd like to do bullets for lists, and there is xmldoc syntax for this, but no major editor that I can see translates the <item> or <list> elements nicely :-/.

I do agree that making it easier to write good, compatible comments would be great. Or contribute a markdown frontend that the compiler would translate from MD->XML for tooling 🧌

@MangelMaxime
Copy link
Contributor

After thinking about it, we should indeed aim for better compatibility.

What is the or element you speak about @baronfel ?

@baronfel
Copy link
Contributor Author

@MangelMaxime sorry, that was a formatting issue with my message. I was meaning the list and/or item elements mentioned in the C# documentation comments pages. And yes those are C# docs, but I think it's reasonable to support those.

@baronfel
Copy link
Contributor Author

Boo, I guess looking at the actual F# docs those tags aren't supported/recognized:
https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/xml-documentation#recommended-tags

@MangelMaxime
Copy link
Contributor

I suspect F# XML doc to be a subset of C# XML documentation.

And because sometime we are accessing C# libraries it make sense to support everything :) And sure supporting the list items should not be difficult even supporting the type bullet etc should be doable depending on what VSCode allow us to do.

I wanted to look again at the XML/Markdown formatter because I saw some regression (or bugs?) in it compare to what I remembered from Ionide 3.

@Krzysztof-Cieslak Can I try looking at updating the auto generated doc comments to use XML instead of Markdown?

@Krzysztof-Cieslak
Copy link
Member

I;m fairly sure F# support exactly the same XML Docs tags as C#, just this documentation is not complete CC: @cartermp

@Krzysztof-Cieslak
Copy link
Member

@MangelMaxime, sure, go ahead.

@baronfel
Copy link
Contributor Author

baronfel commented Jun 24, 2019

Sample code and how tips appear:

module Say =
    ///<summary>here is some summary</summary>
    ///<remarks>
    /// with remarks
    ///<list type="bullet">
    ///<item>and here is an item</item>
    ///</list>
    ///</remarks>
    let hello name =
        printfn "Hello %s" name

ionide 4:
image

Rider 2019.1.2:
image

And I couldn't get a build/tooltip out of VS for Mac at all.

It seems like there are a few pieces here:

  • enhance Ionide/FSAC to supply default documentation that is in xml-doc-comment format
    • useful, good across all editors, definitely should do
  • enhance ionide/fsac without core compiler updates to handle things like list/item/etc.
    • possibly useful in the short term until compiler/FCS changes go in?
    • might result in people writing xmldoc that other editors don't show well
  • enhance compiler/FCS to learn about tags that it doesn't currently support
    • best IMO because then other editors can participate so we all have better xmldoc support.

Thoughts?

@MangelMaxime
Copy link
Contributor

enhance Ionide/FSAC to supply default documentation that is in xml-doc-comment format

Yes, this is what I will be working at first

enhance Ionide/FSAC to supply default documentation that is in xml-doc-comment format

I will make a pass on this one too when generation is ok. Like that I will be able to test how things looks :)

enhance compiler/FCS to learn about tags that it doesn't currently support

I think FSAC is reading the doc comments both from FCS when it in your corrent code and the xml file when it's a package.

FCS gives back the raw XML which is normal because it can't assume the language use for formatting the code. I don't think there is much to do here.

@MangelMaxime
Copy link
Contributor

Update we have a PR #1175 ready for generating XML comment instead of markdown.

I am now taking a look to support good display for all the XML comment features.

@baronfel
Copy link
Contributor Author

I love it so far, you're wonderful for tackling this.

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Jul 12, 2019

I opened an issue to see if we could allow "Command URL" in the tooltip.

This would allow us to open the InfoPanel in order to jump to type documentation.

microsoft/vscode-languageserver-node#503

@MangelMaxime
Copy link
Contributor

@Krzysztof-Cieslak

I am working on the Tooltip improvements and I need helps to take decisions.

So, here is the situation:

  • I first worked on FSAC markdown helpers and I have them supporting the complete feature sets of XML docs
  • However, when parsing doc comments from a library we are using another parser FSAC tip formatter

So we have two implementations.

  • But we also have an implementation of the parser hosted in Ionide itself.

Can we remove the Ionide implementation, I think it could be handled inside FSAC no?

If we do so, we could decide to have only one implementation in FSAC. Because handling one implementation is already hard enough. The docs format are hard enough to implement correctly in one parser because the XML comments are not consistent some are using 2, 3, 4 spaces indentation, the XML specials chars are not always escaped, etc.

@Krzysztof-Cieslak
Copy link
Member

  1. Yes, markdown module need to go away, I think we use it only in Info Panel, but we should be able to apply same transformations on FSAC side

  2. I think I'd like to keep the TipFormatter - it seems it's doing bit more stuff (support for tables, fixes for broken netstandard.xml) - I believe this is the case we hit for user code comment - https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/TipFormatter.fs#L229. We probably should push the string returned here through formatting and it should be fine.

@MangelMaxime
Copy link
Contributor

  1. Yes, markdown module need to go away, I think we use it only in Info Panel, but we should be able to apply same transformations on FSAC side

I agree, will include the changes for it in my PR.

  1. I think I'd like to keep the TipFormatter - it seems it's doing bit more stuff (support for tables, fixes for broken netstandard.xml) - I believe this is the case we hit for user code comment - https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/TipFormatter.fs#L229. We probably should push the string returned here through formatting and it should be fine.

I can port the fixed from the Markdown module to TipFormatter which don't do everything correctly. And yes, we got extra stuff in the tip formatter but I have no idea why. Because tables are not a feature of doc comments in theory and stuff like that.

The only drawback of TipFormatter is because it's working on XML it sensitive to non escaped XML chars <. In theory user should never write < in his comment but &lt; so that's "fine".

@Krzysztof-Cieslak
Copy link
Member

Because tables are not a feature of doc comments in theory and stuff like that.

Yeah, well, you can use them inside documentation section. And they're used in practice, even by MSFT:
image

@Krzysztof-Cieslak
Copy link
Member

I can port the fixed from the Markdown module to TipFormatter which don't do everything correctly

Yeah, let's do that.

@MangelMaxime
Copy link
Contributor

@Krzysztof-Cieslak Thank you for giving me a test for table support :)

Current state of the tooltip:

2019-07-25 15 44 52

It demonstrates all the XML comment feature except:

  • bullet lists
  • table support

We support an extra feature for code tags. You can provide an optional lang attribute to get syntax highlights.

For example: <code lang="fsharp">, <code lang="csharp">

Of course, I will mention it in the FSAC PR and so maintainers can decide if they do want this extra feature or no.

@MangelMaxime
Copy link
Contributor

Ok so...

Microsoft is using tags, property and syntax that aren't documented...

For example:

<member name="M:System.Uri.#ctor(System.String)">
	<summary>Initializes a new instance of the 
		<see cref="T:System.Uri"></see> class with the specified URI.
	</summary>
	<param name="uriString">A URI.</param>
	<exception cref="T:System.ArgumentNullException">
		<paramref name="uriString">uriString</paramref> is null.
	</exception>
	<exception cref="T:System.UriFormatException">
		<block subset="none" type="note">
       In the 
			<a href="http://go.microsoft.com/fwlink/?LinkID=247912" data-raw-source="[.NET for Windows Store apps](http://go.microsoft.com/fwlink/?LinkID=247912)" sourcefile="netstandard.yml" sourcestartlinenumber="3" sourceendlinenumber="3">.NET for Windows Store apps</a> or the 
			<a href="~/docs/standard/cross-platform/cross-platform-development-with-the-portable-class-library.md" data-raw-source="[Portable Class Library](~/docs/standard/cross-platform/cross-platform-development-with-the-portable-class-library.md)" sourcefile="netstandard.yml" sourcestartlinenumber="3" sourceendlinenumber="3">Portable Class Library</a>, catch the base class exception, 
			<xref href="System.FormatException"></xref>, instead.  

    
		</block>
		<code data-dev-comment-type="paramref">uriString</code> is empty.  
 -or-  
 The scheme specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not correctly formed. See 
		<xref href="System.Uri.CheckSchemeName(System.String)"></xref>.  
 -or-  
 
		<code data-dev-comment-type="paramref">uriString</code> contains too many slashes.  
 -or-  
 The password specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not valid.  
 -or-  
 The host name specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not valid.  
 -or-  
 The file name specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not valid.  
 -or-  
 The user name specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not valid.  
 -or-  
 The host or authority name specified in 
		<code data-dev-comment-type="paramref">uriString</code> cannot be terminated by backslashes.  
 -or-  
 The port number specified in 
		<code data-dev-comment-type="paramref">uriString</code> is not valid or cannot be parsed.  
 -or-  
 The length of 
		<code data-dev-comment-type="paramref">uriString</code> exceeds 65519 characters.  
 -or-  
 The length of the scheme specified in 
		<code data-dev-comment-type="paramref">uriString</code> exceeds 1023 characters.  
 -or-  
 There is an invalid character sequence in 
		<code data-dev-comment-type="paramref">uriString</code>.  
 -or-  
 The MS-DOS path specified in 
		<code data-dev-comment-type="paramref">uriString</code> must start with c:\\.

	</exception>
</member>

I should have never looked at netstandard.xml 🙄

@Krzysztof-Cieslak
Copy link
Member

Yes, they also have places where it's not valid XML in netstandard.xml (there is workaround for some of that in Tip Formatter)

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Jul 26, 2019

Update of the day :)

Today I added support for -or- syntax.

Example taken from System.Uri("") constructor.

<?xml version="1.0" encoding="utf-8"?>
<doc>
<assembly><name>testLibrary</name></assembly>
<members>
<member name="P:TestLibrary.testEverything">
 <summary></summary>
 <exception cref="T:System.UriFormatException"><block subset="none" type="note">
        In the <a href="http://go.microsoft.com/fwlink/?LinkID=247912" data-raw-source="[.NET for Windows Store apps](http://go.microsoft.com/fwlink/?LinkID=247912)" sourcefile="netstandard.yml" sourcestartlinenumber="3" sourceendlinenumber="3">.NET for Windows Store apps</a> or the <a href="~/docs/standard/cross-platform/cross-platform-development-with-the-portable-class-library.md" data-raw-source="[Portable Class Library](~/docs/standard/cross-platform/cross-platform-development-with-the-portable-class-library.md)" sourcefile="netstandard.yml" sourcestartlinenumber="3" sourceendlinenumber="3">Portable Class Library</a>, catch the base class exception, <xref href="System.FormatException"></xref>, instead.

     </block>
     <code data-dev-comment-type="paramref">uriString</code> is empty.
  -or-
  The scheme specified in <code data-dev-comment-type="paramref">uriString</code> is not correctly formed. See <xref href="System.Uri.CheckSchemeName(System.String)"></xref>.
  -or-
  <code data-dev-comment-type="paramref">uriString</code> contains too many slashes.
  -or-
  The password specified in <code data-dev-comment-type="paramref">uriString</code> is not valid.
  -or-
  The host name specified in <code data-dev-comment-type="paramref">uriString</code> is not valid.
  -or-
  The file name specified in <code data-dev-comment-type="paramref">uriString</code> is not valid.
  -or-
  The user name specified in <code data-dev-comment-type="paramref">uriString</code> is not valid.
  -or-
  The host or authority name specified in <code data-dev-comment-type="paramref">uriString</code> cannot be terminated by backslashes.
  -or-
  The port number specified in <code data-dev-comment-type="paramref">uriString</code> is not valid or cannot be parsed.
  -or-
  The length of <code data-dev-comment-type="paramref">uriString</code> exceeds 65519 characters.
  -or-
  The length of the scheme specified in <code data-dev-comment-type="paramref">uriString</code> exceeds 1023 characters.
  -or-
  There is an invalid character sequence in <code data-dev-comment-type="paramref">uriString</code>.
  -or-
  The MS-DOS path specified in <code data-dev-comment-type="paramref">uriString</code> must start with c:\\.
 </exception>
</member>
</members>
</doc>

Here is my proposition:

I first show only a -or- example and then I show how it looks like when inside a "real/complete" tooltip.

See how now the exception item detect if it should be rendered on a single line or using multiples lines.

xml_or_demo

Here is how Microsoft is rendering it on its website:

image

My question, is are you ok with my proposition? I intentionally added an extra indentation level and kept the or word. The former being to make it easier to distinguish the "section" and the latter so the user knows this is a list of possibilities.

@MangelMaxime
Copy link
Contributor

For info, I am moving the conversation in the FSAC PR: ionide/FsAutoComplete#446

Now that I have almost everything in it, it will make it easier to discuss with others maintainers etc. And also, now the implementation is public so I can use it as an example to explains things if needed.

@Krzysztof-Cieslak
Copy link
Member

Well, this was done a few versions ago. Huge props to @MangelMaxime for working on it.

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

No branches or pull requests

4 participants