Azure / autorest.csharp

Extension for AutoRest (https://github.com/Azure/autorest) that generates C# code
MIT License
141 stars 166 forks source link

Support overriding XML doc comments for methods and properties #2376

Open heaths opened 2 years ago

heaths commented 2 years ago

Whether using HLC codegen or DPG, there have been cases where we've wanted to improve the documentation that gets generated beyond simply transforming the swagger description. It'd be great if there was a way to do this, though off hand I'm not sure what that would look like so we rely heavily on Roslyn, which would need some stub - like a property or method - to hang this information on.

For properties we can do that fairly easily today, at least. Most often, we just need to redeclare the property and regenerate, and we can add whatever docs we want.

For methods, though, we have to define the implementation and lose all the goodness of fully-generated code, which then we have to keep up to date manually or miss potentially critical changes to code gen.

It's hacky, but perhaps using a partial void method with the same parameters to match which, if not implemented, would be elided by the compiler. Should show up in Roslyn, though, so the code generator could pick up any doc comments. Just a thought, and I'm sure there's more.

heaths commented 2 years ago

To note, Python has something this in their patch support via a __patch.py file where they can assign new docs to objects' __doc__ property. Clearly that wouldn't work for us, but the idea of duck typing something like the method I mentioned above - with a name like private partial void {MethodGroupNameSansAsync}Docs - could work for us and still support Roslyn.

AlexanderSher commented 2 years ago

@heaths, would replacing of the top level elements be enough for this one?

heaths commented 2 years ago

You mean like taking full control of <summary>, <parameter>, <remarks>, etc.? Possibly. My main concern there for DPG is for generated schemas for protocol clients. Off the top of my head, maybe an idea is a placeholder where DPG could still inject that generated schema, like <generatedSchema/> we could use? Just a thought.

archerzz commented 2 years ago

IMHO, it would be better to handle that in post-build stages, since the requirement of overriding could be flexible so that the workaround using Rosolyn could be very hack (thus hard to maintain) or even impossible.

The dotnet build will compile the comments into an XML document, like below:

<?xml version="1.0" encoding="utf-8"?>
<doc>
    <assembly>
        <name>Azure.Analytics.Purview.Scanning</name>
    </assembly>
    <members>
        <member name="M:Azure.Analytics.Purview.Scanning.PurviewClassificationRuleClient.#ctor(System.Uri,System.String,Azure.Core.TokenCredential)">
            <summary> Initializes a new instance of PurviewClassificationRuleClient. </summary>
            <param name="endpoint"> The scanning endpoint of your purview account. Example: https://{accountName}.scan.purview.azure.com. </param>
            <param name="classificationRuleName"> The String to use. </param>
            <param name="credential"> A credential used to authenticate to an Azure Service. </param>
            <exception cref="T:System.ArgumentNullException"> <paramref name="endpoint" />, <paramref name="classificationRuleName" /> or <paramref name="credential" /> is null. </exception>
            <exception cref="T:System.ArgumentException"> <paramref name="classificationRuleName" /> is an empty string, and was expected to be non-empty. </exception>
        </member>

So theoretically we can use XML tools to apply a patch file to the XML doc team. I'm asking doc team on advice.

archerzz commented 2 years ago

@heaths doc team recommend to directly update the generated XML doc (like this), and add overwrite="false" attribute to the corresponding XML nodes, so that the change will not be overwritten by daily pipeline.

heaths commented 2 years ago

This needs to be built into our pipeline then. It's not feasible to ask every SDK to figure out their own way. Who's responsible for that? /cc @jsquire

jsquire commented 2 years ago

The docs process and pipeline are owned by the EngSys team. To my knowledge, @danieljurek would be the right person to start a conversation with.

heaths commented 2 years ago

@jsquire this isn't just about external docs, though. This is also about our XML doc comments e.g., that IntelliSense shows. Meaning, however customizations are handled, it needs to happen during build prior to packing - not the release pipeline.

chunyu3 commented 2 years ago

@AlexanderSher will discuss with @heaths .

heaths commented 2 years ago

Talking with @m-nash offline, I wanted to clarify a few things:

  1. This is not blocking. It's a nice-to-have feature ask.
  2. It may not be worth spending time on it for swagger-based generators.
  3. It'd probably be even easier in Cadl, with decorators (just off the cuff naming) like @details, @example, etc.; though, something language-specific (and rather large, most likely) like @example might be better in a per-language sidecar file.
  4. The intent of this was to supply my own <remarks> or <example> or whatever in C#, which will vary in other languages, in a way that is language-agnostic without having to declare (effectively "override" the generator) the member. For properties this isn't a big deal, but for methods - where the generator writes a lot of its code - would be a huge deal to "override" that and take full ownership.