Add document for ambiguity handling #37795

pshao25 · 2023-07-24T02:53:47Z

Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

doc/DataPlaneCodeGeneration/Handle_Ambiguity.md

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

github-actions · 2023-11-03T05:04:41Z

Hi @pshao25. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

github-actions · 2023-11-10T08:31:27Z

Hi @pshao25. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

chunyu3

What's audience of this doc? the SDK users or codegen?
What's the main purpose of this doc? To describe how to resolve ambiguity or show the protocol/convenience methods generated?
We may need to rearrange the content and the chapter according to the audience and purpose.

annelo-msft

I love @chunyu3's comment #37795 (review)!

To add to @chunyu3's thoughts, it might be helpful to think about how this document works in the context of larger documentation efforts. I know @jsquire and @scottaddie have been thinking about this, and may be able to help describe the overarching effort here.

I'm also curious ... is this what we're doing today, or does it take into account future plans described by @KrzysztofCwalina in System.ClientModel Method Conventions (github.com)

pshao25 · 2023-11-30T05:41:50Z

@annelo-msft @chunyu3 The audience are the codegen users (i.e., service team) who are curious about the logic how the signature of method is generated.

it might be helpful to think about how this document works in the context of larger documentation efforts

For sure. But seems customer is asking for this document so I think we could continue this PR, and after we decide how to refactor we could move this page to that efforts.

is this what we're doing today, or does it take into account future plans

This is what we are doing today. It doesn't take any non-branding cases into account.

github-actions · 2024-02-02T05:04:43Z

Hi @pshao25. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

github-actions · 2024-02-09T08:31:14Z

Hi @pshao25. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

…o ambiguityDoc

ArcturusZhang · 2024-11-12T06:28:59Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+- If the protocol method hasn’t been GAed, the first solution is taken if there exists an ambiguous call.
+- If the protocol method is already GAed, the second solution is taken because we cannot change protocol method anymore.
+
+## Scope of ambiguity we handle


this standalone section of "edge cases" is making the following content a bit off-topic.
Could we make it a note of the section of Solution?

ArcturusZhang · 2024-11-12T06:32:43Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+
+## Scope of ambiguity we handle
+
+Not all the pairs of protocol method and convenience method will have such problem. Below is a case when there is no ambiguity if users are calling normally by passing a `model` or a `content`. (They could not pass in `null` because it will throw error)


I think here we should summarize this section in case people would fixate on edge cases and ignoring the problem we are trying to solve such as:
It is impossible that the ambiguities could always be fixed when the two methods are overloads of each other, because of the existence of default. We could always use the default keyword which is acceptable by any parameter to construct a scenario that the compiler would complain about ambiguities of invocation. Therefore our goal is not solving all cases of the ambiguities, for the following cases, we will allow them to have ambiguities: ...

And I see we have two principals in this section, we should emphasize them first and then have examples.

ArcturusZhang · 2024-11-12T06:33:30Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

@@ -0,0 +1,225 @@
+# Handle Ambiguity Between Protocol Method and Convenience Method
+
+## Background


instead of Background, i think this is more of a Problem corresponding to the Solution we have below.

ArcturusZhang · 2024-11-12T06:35:32Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+Then we find these calls are fine, so just keep these signatures.
+
+## Appendix
+### Convenience method not always generated


Suggested change

### Convenience method not always generated

### Convenience methods are not always generated

ArcturusZhang · 2024-11-12T06:36:38Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+    ...
+}
+```
+The convenience method has a required parameter `requiredId` and protocol method has a required method `content`, so what we guarantee will not have ambiguity are


Suggested change

The convenience method has a required parameter `requiredId` and protocol method has a required method `content`, so what we guarantee will not have ambiguity are

The convenience method has a required parameter `requiredId` and protocol method has a required parameter `content`, therefore we guarantee the following invocations will not have ambiguities:

ArcturusZhang · 2024-11-12T06:39:32Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+
+## Examples
+
+### Operation without input


the word input is quite ambiguous. requiredId is also an input in the following example.
Should it be Operations without request body?

ArcturusZhang · 2024-11-12T06:40:28Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+}
+```
+We don't generate convenience method for this case (see [this](#convenience-method-not-always-generated) to see why), so there will be no ambiguity.
+### Operation with spread scenario


Suggested change

### Operation with spread scenario

### Operation with spread parameters

ArcturusZhang · 2024-11-12T06:41:30Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+
+## Appendix
+### Convenience method not always generated
+There is one [situation](#operation-without-both-input-and-output) we don't generate convenience method. Think about what would be like if we generate the convenience method.


Suggested change

There is one [situation](#operation-without-both-input-and-output) we don't generate convenience method. Think about what would be like if we generate the convenience method.

There is one [situation](#operation-without-both-input-and-output) we don't generate convenience method, because if we did, the convenience method would have almost exact the same parameter list as the protocol method:

ArcturusZhang · 2024-11-12T06:42:16Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+    ...
+}
+```
+The convenience method is just a wrapper and doesn't have any value. So, we just skip generating it.


Suggested change

The convenience method is just a wrapper and doesn't have any value. So, we just skip generating it.

The convenience method is just a wrapper therefore it is not generated.

ArcturusZhang · 2024-11-12T06:44:10Z

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md

+### Special values not taken into ambiguity consideration
+Think about these totally valid overloads that you've seen many times in your daily development.
+```C#
+class IntModel
+{
+    public int Value {get; set;}
+}
+
+class DoubleModel
+{
+    public double Value {get; set;}
+}
+
+int Add(IntModel a, IntModel b)
+{
+    return a.Value + b.Value;
+}
+double Add(DoubleModel a, DoubleModel b)
+{
+    return a.Value + b.Value;
+}
+```
+When calling `Add(null, null)` or `Add(default, default)` or `Add(new(), new())`, ambiguity error still exists. Therefore we don't care about `null`, or `default`, or `new()`.


I do not think this is necessary to explain, or at least it should not be here as an appendix - it is against the normal reading sequence on human. No one would read a document like this.
In the other comment, I have suggested that we put the reason in the above sections as a note.

github-actions · 2025-01-17T05:05:08Z

Hi @pshao25. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

Add document for ambiguity handling

7941456

pshao25 mentioned this pull request Jul 24, 2023

Handle ambiguity Azure/autorest.csharp#3616

Merged

heaths reviewed Jul 25, 2023

View reviewed changes

doc/DataPlaneCodeGeneration/Handle_Ambiguity.md Outdated Show resolved Hide resolved

doc/DataPlaneCodeGeneration/Handle_Ambiguity.md Outdated Show resolved Hide resolved

doc/DataPlaneCodeGeneration/Handle_Ambiguity.md Outdated Show resolved Hide resolved

pshao25 added 2 commits August 29, 2023 11:32

resolve comments

731c761

Update

5b49c8e

pshao25 mentioned this pull request Aug 29, 2023

Figure out the flags for ambiguity call #38405

Closed

archerzz reviewed Aug 29, 2023

View reviewed changes

lirenhe reviewed Aug 29, 2023

View reviewed changes

doc/DataPlaneCodeGeneration/Handle-Ambiguity.md Outdated Show resolved Hide resolved

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Nov 3, 2023

github-actions bot closed this Nov 10, 2023

pshao25 reopened this Nov 29, 2023

pshao25 requested review from chunyu3 and annelo-msft as code owners November 29, 2023 03:02

pshao25 mentioned this pull request Nov 29, 2023

RequestContext changed from optional to required Azure/autorest.csharp#3589

Closed

github-actions bot removed the no-recent-activity There has been no recent activity on this issue. label Nov 29, 2023

chunyu3 reviewed Nov 29, 2023

View reviewed changes

annelo-msft requested review from jsquire, scottaddie and pallavit November 29, 2023 16:27

annelo-msft reviewed Nov 29, 2023

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Feb 2, 2024

github-actions bot closed this Feb 9, 2024

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-net int…

ab0e426

…o ambiguityDoc

pshao25 reopened this Nov 12, 2024

update

6077142

github-actions bot removed the no-recent-activity There has been no recent activity on this issue. label Nov 12, 2024

ArcturusZhang reviewed Nov 12, 2024

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add document for ambiguity handling #37795

Add document for ambiguity handling #37795

pshao25 commented Jul 24, 2023

github-actions bot commented Nov 3, 2023

github-actions bot commented Nov 10, 2023

chunyu3 left a comment

annelo-msft left a comment

pshao25 commented Nov 30, 2023

github-actions bot commented Feb 2, 2024

github-actions bot commented Feb 9, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

ArcturusZhang Nov 12, 2024

github-actions bot commented Jan 17, 2025


		## Scope of ambiguity we handle

		Not all the pairs of protocol method and convenience method will have such problem. Below is a case when there is no ambiguity if users are calling normally by passing a `model` or a `content`. (They could not pass in `null` because it will throw error)

		@@ -0,0 +1,225 @@
		# Handle Ambiguity Between Protocol Method and Convenience Method

		## Background

	### Convenience method not always generated
	### Convenience methods are not always generated

	The convenience method has a required parameter `requiredId` and protocol method has a required method `content`, so what we guarantee will not have ambiguity are
	The convenience method has a required parameter `requiredId` and protocol method has a required parameter `content`, therefore we guarantee the following invocations will not have ambiguities:

	### Operation with spread scenario
	### Operation with spread parameters

	There is one [situation](#operation-without-both-input-and-output) we don't generate convenience method. Think about what would be like if we generate the convenience method.
	There is one [situation](#operation-without-both-input-and-output) we don't generate convenience method, because if we did, the convenience method would have almost exact the same parameter list as the protocol method:

	The convenience method is just a wrapper and doesn't have any value. So, we just skip generating it.
	The convenience method is just a wrapper therefore it is not generated.

Add document for ambiguity handling #37795

Are you sure you want to change the base?

Add document for ambiguity handling #37795

Conversation

pshao25 commented Jul 24, 2023

Contributing to the Azure SDK

github-actions bot commented Nov 3, 2023

github-actions bot commented Nov 10, 2023

chunyu3 left a comment

Choose a reason for hiding this comment

annelo-msft left a comment

Choose a reason for hiding this comment

pshao25 commented Nov 30, 2023

github-actions bot commented Feb 2, 2024

github-actions bot commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025