DPG, partial-update, merge javadoc with user content on client method

Azure / autorest.java

Extension for AutoRest (https://github.com/Azure/autorest) that generates Java code

MIT License

33 stars 82 forks source link

DPG, partial-update, merge javadoc with user content on client method #2510

Open weidongxu-microsoft opened 10 months ago

weidongxu-microsoft commented 10 months ago

case https://github.com/Azure/azure-sdk-for-java/pull/38307 (update, the issue in this case is on Class, hence covered by https://github.com/Azure/autorest.java/issues/2492)

Client/ClientBuilder would be this issue https://github.com/Azure/autorest.java/issues/2492

Below is for client method.

First see if we can "merge" the Javadoc description section.

May use the same   solution.

Codegen be

 * <!-- start generated doc -->
 * Initializes a new instance of the synchronous ImageAnalysisClient type.
 * <!-- end generated doc -->

embed be

 * <!-- src_embed com.azure.ai.vision.imageanalysis.sync-client -->
 * <pre>
 * &#47;&#47;
 * &#47;&#47; Create a synchronous Image Analysis client.
 * &#47;&#47;
 * ImageAnalysisClient client = new ImageAnalysisClientBuilder&#40;&#41;
 *     .endpoint&#40;endpoint&#41;
 *     .credential&#40;new KeyCredential&#40;key&#41;&#41;
 *     .buildClient&#40;&#41;;
 * </pre>
 * <!-- end com.azure.ai.vision.imageanalysis.sync-client -->

and combine it to

 * <!-- start generated doc -->
 * Initializes a new instance of the synchronous ImageAnalysisClient type.
 * <!-- end generated doc -->
 *
 * <!-- src_embed com.azure.ai.vision.imageanalysis.sync-client -->
 * <pre>
 * &#47;&#47;
 * &#47;&#47; Create a synchronous Image Analysis client.
 * &#47;&#47;
 * ImageAnalysisClient client = new ImageAnalysisClientBuilder&#40;&#41;
 *     .endpoint&#40;endpoint&#41;
 *     .credential&#40;new KeyCredential&#40;key&#41;&#41;
 *     .buildClient&#40;&#41;;
 * </pre>
 * <!-- end com.azure.ai.vision.imageanalysis.sync-client -->

### Tasks
- [ ] refactor existing code to seperate javadoc description and tags calls
- [ ] codegen change to add the tags to both class and method level javadoc, release autorest.java, and sync typespec related RPs
- [ ] run the script to update DPG and vanilla parts, include both class level and method level
- [ ] codegen change to handle partial update on javadoc

weidongxu-microsoft commented 10 months ago

Another is where do we put the start and end.

E.g. for a full Javadoc, should we do this

    /**
     * <!-- start generated doc -->
     * The read operation.
     * <!-- end generated doc -->
     * 
     * @param filter A sequence of textual characters.
     * @throws IllegalArgumentException thrown if parameters fail the validation.
     * @throws HttpResponseException thrown if the request is rejected by server.
     * @throws ClientAuthenticationException thrown if the request is rejected by server on status code 401.
     * @throws ResourceNotFoundException thrown if the request is rejected by server on status code 404.
     * @throws ResourceModifiedException thrown if the request is rejected by server on status code 409.
     * @throws RuntimeException all other wrapped checked exceptions if the request fails to be sent.
     * @return the response.
     */

or this

    /**
     * <!-- start generated doc -->
     * The read operation.
     * 
     * @param filter A sequence of textual characters.
     * @throws IllegalArgumentException thrown if parameters fail the validation.
     * @throws HttpResponseException thrown if the request is rejected by server.
     * @throws ClientAuthenticationException thrown if the request is rejected by server on status code 401.
     * @throws ResourceNotFoundException thrown if the request is rejected by server on status code 404.
     * @throws ResourceModifiedException thrown if the request is rejected by server on status code 409.
     * @throws RuntimeException all other wrapped checked exceptions if the request fails to be sent.
     * @return the response.
     * <!-- end generated doc -->
     */

haolingdong-msft commented 10 months ago

Design discussion on 2024-1-18

Option 1:

Add <start> and <end> tags for generated javadocs, similar as current package-info.java. We will have a single block for generated comments. We will overwrite the javadoc within <start> and <end> wrapper. If the user wants to add in the middle of <start> and <end> wrapper, they will need to maintain the javadoc by themselves by removing the <start> and <end> tags. How to do backfill? Have script to go through the repo to update the method's javadoc.

Cons: Need to do backfill to update existing method's javadoc to include the tags, otherwise in the future we are not able to tell if the javadoc is user partially updated or generated.

Option 2：

Add customized tags. <customized start><customized end>. We can only allow customization at the top of javadoc description. Otherwise it's very hard to replace the generated javadoc.

Cons: Have to rely on user to add the tags. Can only allow customization at the top of javadoc.

weidongxu-microsoft commented 10 months ago

Sketch of the script in option1

iterate all Java files in repo
if no @Generated find in file, skip
add start end to class Javadoc
for each client method, if it has @Generated, add start end to method Javadoc

It may actually less than we expected, as vanilla probably don't have @Generated in class.

DPG from TypeSpec is not a concern, as each time we bump typespec-java, we refresh the whole repo for those SDKs.

haolingdong-msft commented 10 months ago

vanilla has @Generated annotation on builders: https://github.com/Azure/autorest.java/blob/main/vanilla-tests/src/main/java/fixtures/additionalproperties/AdditionalPropertiesClientBuilder.java#L61

weidongxu-microsoft commented 10 months ago

script to collect current info https://github.com/weidongxu-microsoft/autorest.java/blob/tmp_script/eng/sdk/update_javadoc.py (it does not update anything, yet)

$ python eng/sdk/update_javadoc.py --sdk-root /c/github/azure-sdk-for-java

azure-verticals-agrifood-farming, count 111
azure-ai-anomalydetector, count 3
azure-communication-email, count 1
azure-communication-jobrouter, count 127
azure-communication-phonenumbers, count 22
azure-communication-rooms, count 1
azure-security-confidentialledger, count 6
azure-ai-contentsafety, count 23
azure-developer-devcenter, count 9
azure-iot-deviceupdate, count 6
azure-ai-documentintelligence, count 75
azure-analytics-defender-easm, count 99
azure-messaging-eventgrid, count 1
azure-health-insights-cancerprofiling, count 3
azure-health-insights-clinicalmatching, count 3
azure-developer-loadtesting, count 6
azure-maps-elevation, count 1
azure-maps-geolocation, count 1
azure-maps-render, count 1
azure-maps-route, count 2
azure-maps-search, count 1
azure-maps-timezone, count 1
azure-maps-traffic, count 1
azure-maps-weather, count 3
azure-ai-metricsadvisor, count 1
azure-monitor-ingestion, count 3
azure-monitor-query, count 5
azure-ai-openai, count 106
azure-ai-personalizer, count 1
azure-analytics-purview-administration, count 15
azure-analytics-purview-catalog, count 21
azure-analytics-purview-scanning, count 19
azure-analytics-purview-sharing, count 9
azure-analytics-purview-workflow, count 27
azure-messaging-servicebus, count 1
azure-spring-data-cosmos, count 5
azure-storage-blob, count 1
azure-storage-file-datalake, count 1
azure-storage-file-share, count 1
azure-storage-queue, count 1
azure-analytics-synapse-artifacts, count 49
azure-data-tables, count 1
azure-ai-textanalytics, count 1
azure-ai-documenttranslator, count 3
azure-ai-translation-text, count 3
azure-ai-vision-imageanalysis, count 24

Seems manageable for option1

package having lots of it would be typespec (these should be covered in the next regen on typespec-java update, if we support option1)
package having some of it would be DPG ("azure-verticals-agrifood-farming" actually here, they got lots of Client/Builder)
package having 1 or few of it could be vanilla (as Haoling mentioned, Builder has the annotation)

(latter 2 can be covered via script, probably at level about modifying 100+ file)

alzimmermsft commented 10 months ago

I'm in favor of option 1 as it doesn't require someone to look up a custom XML comment to add in additional custom Javadocs. It's very possible for the person generating code and the person maintaining it later to be different people, and having to find some custom XML comment to add custom Javadocs may be hard to find.

I would though make the XML comment itself a bit more descriptive than <start> and <end>, such as <generated-javadoc-start> and <generated-javadoc-end>. While it's way more verbose, it also clearly explains what it is. And, if we really feel like it, since these are all XML comments, we could have an XML comment linking out to more details about how it works.

Ex:

/**
   * <!-- generated-javadoc-start -->
   * <!-- Generated comment linking out to more details or having more details inline. -->
   * The generated Javadoc.
   * <!-- generated-javadoc-end -->
   */ 
public class MyGeneratedClass {
}

weidongxu-microsoft commented 10 months ago

I am also lean toward option1, as it more consistent with other situations (that codegen adding token like @Generated, not user adding "customize" token).

Alan's suggestion on comment tag also make sense. I bit uncertain about a link (it may get outdated if not maintained, and it seems appear too many places, maybe just class javadoc? -- anyway if we use it would be an aka link).

haolingdong-msft commented 10 months ago

Summarize our discussions so far and add my preference:

Option1 would be consistent with current logic.
Option1 backfill is manageble, we will add javadoc tags <generated-javadoc-start> and <generated-javadoc-end> on the client method for the files that has @Generated annotation. This includes typespec, DPG and vanilla(only Builder).
Only allow partial update on the javadoc descriptions, not allow partial update on parameters. If they want to update on parameters, they need to remove the tags and maintain the javadoc by themselves. Add and tags around whole javadoc.
If the user wants to add customized javadoc in the middle of and wrapper, they will need to maintain the javadoc by themselves by removing the and tags.

I think as backfill is manageble, option1 would be suitable.

More complex case: generated version:

/** 
<generated-javadoc-description-start>
typespec desc
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@parameters
<generated-javadoc-tags-end>
**/

after partial update:

/** 
<generated-javadoc-description-start>
typespec desc
<generated-javadoc-description-end>

customized desc
codesnippet

<generated-javadoc-tags-start>
@parameters
<generated-javadoc-tags-end>

**/

haolingdong-msft commented 10 months ago

Implementation steps:

codegen change to add the tags, release autorest.java, and sync typespec related RPs
run the script to update DPG and vanilla parts
codegen change to handle partial update on javadoc

weidongxu-microsoft commented 10 months ago

About the case (that to do when in same case the Javadoc can contain no Javadoc tag)

existing (no user)

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
<generated-javadoc-tags-end>
**/
@Generated
public Builtin() {

generated code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
**/
@Generated
public Builtin(String name) {

Question: do we write this empty

<generated-javadoc-tags-start>
<generated-javadoc-tags-end>

block?

With this empty block, it would be easier for partial-update merge logic.

haolingdong-msft commented 10 months ago

I think it is better to have the empty block, so that later if there are javadoc tags section generated, the empty block would be replaced by the newly generated javadoc.

haolingdong-msft commented 10 months ago

An open question to discuss:

To summarize, currently we have four cases, for marker, I mean <xxx-start> and <xxx-end>:

existing file has no tag/description section (also no markers) -> add generated javadoc
existing file has whole generated tag/description sections with markers, no customized code -> replace the generated content inside markers
existing file has customized tag/description section and remove markers -> keep customized tag/description section
existing file has customized tag/description section and have markers -> keep customized tag/description section and replace the generated content inside markers

I think the javadoc partial update merge logic could be:

if no tag/desc section found in existing file(this can be done using javaparser), just return generated javadoc tags/desc as the after-partial-update javadoc
if have tag/desc section found in existing file, keep the customized code and replace the generated content inside markers(if there is marker)

@weidongxu-microsoft and have difference on case1, if there is no tag/description section, e.g. no javadoc like toJson(), or no javadoc tags section like some methods in Builder, do we treat it as it is never generated, or treat it as user delete it by intension?

If we treat it as never generated, later if we add the javadoc for those client methods in codegen, we will need to add the generated javadoc. -> If we treat it as this, we don't need to add markers for empty javadoc sections.
If we treat it as user delete it by intension, we will not add the generated javadoc. -> If we treat it as this, the partial-update logic could be easier, the first one can even be removed. But we need to add the markers to empty javadoc sections, otherwise there is no way to distinguish if the section is deleted by intension of not.

Would like to hear all your thoughts. @alzimmermsft @srnagar @XiaofeiCao
Thanks!

weidongxu-microsoft commented 10 months ago

On 3 and 4, what is your logic to decide "existing file has customized tag/description section and remove markers"?

If it is

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
@deprecated
@see ABC
**/
@Generated
public Builtin(String name) {

which item is this in?

weidongxu-microsoft commented 10 months ago

I don't think I am actually talking about case1.

I am talking about what's your merge logic to existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

@param name name of the class customized
**/
@Generated
public Builtin(String name) {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>
**/
@Generated
public Builtin() {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
@see ABC
**/
@Generated
public Builtin(String name) {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

@see ABC
**/
@Generated
public Builtin() {

when generated code is

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
**/
@Generated
public Builtin(String name) {

alzimmermsft commented 10 months ago

I like the direction on splitting Javadocs into its two components, description and tags, as different partial update concepts. I do have some more questions on the tags portion though, what happens if someone wants to update a few of the tags:

Initial generated code

/**
   * <!-- generated-javadoc-description-start -->
   * Generated Javadocs.
   * <!-- generated-javadoc-description-end -->
   * 
   * <!-- generated-javadoc-tags-start -->
   * @param val1 generated val1 tag
   * @param val2 generated val2 tag
   * @return generated return tag
   * <!-- generated-javadoc-tags-end -->
   */ 
@Generated
public Return someMethod(String val1, int val2) {

Customized code

/**
   * <!-- generated-javadoc-description-start -->
   * Generated Javadocs.
   * <!-- generated-javadoc-description-end -->
   * <p>
   * Some more custom Javadoc descriptions.
   * 
   * <!-- generated-javadoc-tags-start -->
   * @param val1 generated val1 tag
   * <!-- generated-javadoc-tags-end -->
   * @param val2 custom val2 tag
   * <!-- generated-javadoc-tags-start -->
   * @return generated return tag
   * <!-- generated-javadoc-tags-end -->
   */
@Generated
public Return someMethod(String val1, int val2) {

Would this be something we want to support?

Or what if the developer did this

/**
   * <!-- generated-javadoc-description-start -->
   * Generated Javadocs.
   * <!-- generated-javadoc-description-end -->
   * <p>
   * Some more custom Javadoc descriptions.
   * 
   * @param val2 custom val2 tag
   *<!-- generated-javadoc-tags-start -->
   * @param val1 generated val1 tag
   * @return generated return tag
   * <!-- generated-javadoc-tags-end -->
   */
@Generated
public Return someMethod(String val1, int val2) {

Would this also be something that would work?

Overall, I think supporting both description and tags customizations is worthwhile, I just wonder if some of the more nuanced tags scenarios are worth handling. Or if tags need different pattern.

To answer some thoughts on how to handle updates, I think the main question is about overridden methods that don't have Javadocs as they are inheriting them from the super class. In this case I would treat them as having imaginary generated tags even though they are empty, if Javadocs have been added I'd retain than as if the developer deleted out all the generated tags. And a side question, do we need to continue having the ineheritDoc tag in these cases?

/**
  * {@inheritDoc}.
  */
@Generated
@Override

Could we just remove that as it's implicit for overridden methods. I think that would simplify this conversation as we would need to worry about:

/**
  * <!-- generated-javadoc-description-start -->
  * {@inheritDoc}.
  * <!-- generated-javadoc-description-end -->
  * Custom javadoc.
  */
@Generated
@Override

Also, because adding custom Javadocs in this way is a bit confusing in what the final output would be without generating the Javadoc.

srnagar commented 10 months ago

what happens if someone wants to update a few of the tags:

Tags are tricky to handle. We should simplify and say, if any tags require customization, they should be manually maintained (all of it). The reason is, these tags are mainly for parameters and return types. Both of these docs can be maintained in the TypeSpec description. If further changes are required, we should let the user maintain the whole tags section.

no javadoc like toJson(), or no javadoc tags section like some methods in Builder, do we treat it as it is never generated, or treat it as user delete it by intension?

Our backfill script should still add an empty javadoc section for all methods that currently don't have javadoc tags. For e.g.

/**
  * {@inheritDoc}.
  */
@Generated
@Override

should be updated by the backfill script as

/**
  * <!-- generated-javadoc-description-start -->
  *  {@inheritDoc}.
  *  <!-- generated-javadoc-description-end -->
  * 
  * <!-- generated-javadoc-tag-start -->
  * <!-- generated-javadoc-tag-end-->
  */
@Generated
@Override

Note that we are adding that start and end markers for tags too even though it's currently empty.

haolingdong-msft commented 10 months ago

Or what if the developer did this

If we add

On 3 and 4, what is your logic to decide "existing file has customized tag/description section and remove markers"?

If it is
/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
@deprecated
@see ABC
**/
@Generated
public Builtin(String name) {
which item is this in?

"existing file has customized tag/description section" -> This can be checked by javaparser, in partial update merge logic, we already use javaparser

"remove marker" -> This is done by user.

haolingdong-msft commented 10 months ago

I don't think I am actually talking about case1.

I am talking about what's your merge logic to existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

@param name name of the class customized
**/
@Generated
public Builtin(String name) {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>
**/
@Generated
public Builtin() {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
@see ABC
**/
@Generated
public Builtin(String name) {

or existing code

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

@see ABC
**/
@Generated
public Builtin() {

when generated code is

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
**/
@Generated
public Builtin(String name) {

Merge logic is like this:

if no tag/desc section found in existing file, just return generated javadoc tags/desc as the after-partial-update javadoc
if have tag/desc section found in existing file, keep the customized code and replace the generated content inside markers(if there is marker)

Refer to this comment for more details: https://github.com/Azure/autorest.java/issues/2510#issuecomment-1914042429

haolingdong-msft commented 10 months ago

what happens if someone wants to update a few of the tags:

Tags are tricky to handle. We should simplify and say, if any tags require customization, they should be manually maintained (all of it). The reason is, these tags are mainly for parameters and return types. Both of these docs can be maintained in the TypeSpec description. If further changes are required, we should let the user maintain the whole tags section.

no javadoc like toJson(), or no javadoc tags section like some methods in Builder, do we treat it as it is never generated, or treat it as user delete it by intension?

Our backfill script should still add an empty javadoc section for all methods that currently don't have javadoc tags. For e.g.
/**
  * {@inheritDoc}.
  */
@Generated
@Override
should be updated by the backfill script as
/**
  * 
  *  {@inheritDoc}.
  *  
  * 
  * 
  * 
  */
@Generated
@Override
Note that we are adding that start and end markers for tags too even though it's currently empty.

Hi @srnagar, thanks for the clarification. I still got a question for adding marker for empty block: currently there are some methods don't have javadoc, e.g. toJson(), toJsonMerge(), or no javadoc tags section like some methods in Builder Do we need to add empty markers for them in the codegen logic? e.g. from

@Override
public JsonWriter toJson(JsonWriter jsonWriter) throws IOException {
    jsonWriter.writeStartObject();
    jsonWriter.writeStringField("name", this.name);
    return jsonWriter.writeEndObject();
}

/** 
<generated-javadoc-description-start>
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
<generated-javadoc-tags-end>
**/
@Override
public JsonWriter toJson(JsonWriter jsonWriter) throws IOException {
    jsonWriter.writeStartObject();
    jsonWriter.writeStringField("name", this.name);
    return jsonWriter.writeEndObject();
}

To be consistent with the script, we will need to add the markers for those empty javadocs, otherwise, in next codegen, these empty sections will be treated as customized. Like above example.

But my concerns are:

Firstly, we don't know how many methods have empty javadoc or have empty javadoc tags/desc section. The logic in codegen will be diverse, becuase we need to modify all the places where there is no javadoc, and add the two empty markers for them. Also we could easily miss updating some places. How do we know we cover all method's empty javadocs?
Secondly, next time when someone add logic in codegen, when adding javadoc for a generated method, they need to remember to add the empty markers. Is it a bit overhead?
Thirdly, the generated code will look a bit strange, because the javadoc only contains the meaningless markers.

Instead of add the markers for empty javadoc sections, I list my alternative proposal here: https://github.com/Azure/autorest.java/issues/2510#issuecomment-1914042429

weidongxu-microsoft commented 10 months ago

Why are we talking about "empty block"?

/**
  * {@inheritDoc}.
  */

IS THE JAVADOC. IT IS NOT EMPTY.

And it is autogenerated by codegen, so it should have a start/end on the block.

haolingdong-msft commented 10 months ago

Why are we talking about "empty block"?
/**
  * {@inheritDoc}.
  */
IS THE JAVADOC. IT IS NOT EMPTY.

And it is autogenerated by codegen, so it should have a start/end on the block.

I mean no javadoc tags section. I give examples on some have empty javadoc, some have empty javadoc tags section e.g. https://github.com/Azure/autorest.java/blob/main/typespec-tests/src/main/java/com/_specs_/azure/core/lro/standard/StandardClientBuilder.java#L78, my concern is about the codegen logic: do we need to add markers for this empty javadoc tags section? if we do not add the markers, this empty javadoc tags section will be treated as customized, but it is actually generated. (not sure if there is javadoc has empty description section, but have tags section, if we have, we will need to add markers for empty desc section as well....)

@weidongxu-microsoft and have difference on case1, if there is no tag/description section, e.g. no javadoc like toJson(), or no javadoc tags section like some methods in Builder, do we treat it as it is never generated, or treat it as user delete it by intension?

weidongxu-microsoft commented 10 months ago

I mean no javadoc tags section. I give examples on some have empty javadoc, some have empty javadoc tags section e.g. https://github.com/Azure/autorest.java/blob/main/typespec-tests/src/main/java/com/_specs_/azure/core/lro/standard/StandardClientBuilder.java#L78, my concern is about the codegen logic: do we need to add markers for this empty javadoc tags section? if we do not add the markers, this empty javadoc tags section will be treated as customized, but it is actually generated. (not sure if there is javadoc has empty description section, but have tags section, if we have, we will need to add markers for empty desc section as well....)

@weidongxu-microsoft and have difference on case1, if there is no tag/description section, e.g. no javadoc like toJson(), or no javadoc tags section like some methods in Builder, do we treat it as it is never generated, or treat it as user delete it by intension?

Both here https://github.com/Azure/autorest.java/issues/2510#issuecomment-1911269106 and here https://github.com/Azure/autorest.java/issues/2510#issuecomment-1912626100 already attempted to provide a solution for this question (of empty tag block), isn't it?

weidongxu-microsoft commented 10 months ago

@alzimmermsft

About whether we need to have {@inheritDoc} on overridden method.

I found an old thread about it. https://teams.microsoft.com/l/message/19:5e673e41085f4a7eaaf20823b85b2b53@thread.skype/1585854054501?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1585783051292&teamName=Azure%20SDK&channelName=Language%20-%20Java&createdTime=1585854054501

And I do remember there were some reason we specifically added this block.

On my local check, Javadoc part is good now. I am not sure whether Javadoc fixed it or what... (PS, this seems not released, so I could not get the example from API doc)

We may remove that block, if we are certain this is no longer needed.

haolingdong-msft commented 10 months ago

List my proposals below:

Option1

We will not add additional markers for empty javadoc sections, no matter it's description section, or tags section.
In the partial update merge logic, we will handle first for description section, then for tags section, for each section, do below:
- If no javadoc description/tags section found, we will use the generated javadoc description/tags section as the result javadoc description/tags.
- If javadoc description/tags section found, we will replace the javadoc between the markers and keep the customized javadoc.

Note: We can use com.github.javaparser.JavadocParser.parse() to parse the two sections in a javadoc.

To be more concise, take Weidong's case as example: existing:

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class.
<generated-javadoc-description-end>
**/
@Generated
public Builtin(String name) {

generated:

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class new.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
**/
@Generated
public Builtin(String name) {

The merge logic is executed like below:

Initialize empty result javadoc: String res ="";

check if javadoc description section exists, it do exist, then do the replacement based on the existing javadoc description between the markers. seudo code:


String descJavaDoc = replaceJavaDocFromStartToEndMarkers(existingDescJavaDoc, "<generated-javadoc-description-start>", "<generated-javadoc-description-end>");

/* This is a helper function to replace the javadoc between startMarder to endMarker to replacementJavadoc , and return the result javadoc./ String replaceJavaDocFromStartToEndMarkers(javadoc, replacementJavadoc, startMarker, endMarker)`

3. `res += descJavaDoc ;`
4. check if javadoc tags section exists, it does not exists, then just use the generated javadoc tags section as the result javadoc tags.
`res += generatedTagsJavaDoc`

The result javadoc will be:
```java
/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class new.
<generated-javadoc-description-end>

<generated-javadoc-tags-start>
@param name name of the class
<generated-javadoc-tags-end>
**/
@Generated
public Builtin(String name) {

One case it can not support is: If existing javadoc tags is not empty, but without markers, it will be treated as customized, later if generated javadoc tags add more tags, the generated tags will not be added. e.g.

generate a javadoc:

/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class new.
<generated-javadoc-description-end>
**/
@Generated
public Builtin(String name) {

service team update the javadoc to:


/** 
<generated-javadoc-description-start>
Creates an instance of Builtin class new.
<generated-javadoc-description-end>

@see xxx **/ @Generated public Builtin(String name) {

3. codegen generate a new javadoc:

/**

Creates an instance of Builtin class new.

@param name name of the class **/ ``` 4. After partial update, it is still ```java /** Creates an instance of Builtin class new. @see xxx **/ @Generated public Builtin(String name) { ``` ### **Option2** 1. We will not add additional markers for empty javadoc, but if one javadoc section exists, we will add markers for the other section. 2. In the partial update merge logic, do below: - If empty javadoc found, we will use the generated javadoc to overwrite the empty javadoc. - otherwise, we will replace the javadoc between the markers and keep the customized javadoc. First for description section, then for tags section.

haolingdong-msft commented 9 months ago

2024-2-1

We will first use option2, unless implementation blockers found.
For adding the markers, we need to merge to the feature branch, until backfill is done. But we can first do the refactoring for the existing code and merge to main.