Small nits and some questions #1
Conversation
| @@ -75,7 +75,7 @@ public virtual Response GetPetsByName(string petName, string sort, int? limit, i | |||
| public virtual Response GetPetsByName(string petName, string sort = "ASC", int? limit = 20, int? skip = 0, string species = null, RequestOptions requestOptions = default); | |||
| ``` | |||
|
|
|||
| We can use this strategy to add new optional parameters to methods. Note that since the parameter of a method is always of type `RequestOptions` we can not run into an issue where the new overload creates an ambiguity between different overloads. | |||
| We can use this strategy to add new optional parameters to methods. Note that since the last parameter of a method is always of type `RequestOptions` we cannot run into an issue where the new overload creates an ambiguity between different overloads. | |||
There was a problem hiding this comment.
Is it possible to run into issues if the RequestOptions parameter has a default value?
There was a problem hiding this comment.
It's possible we could run into an issue here (especially for source breaking changes) by making these defaultable, especially now with the thinking around /not/ putting in a cancellation token parameter. I need to think more about that. I think that the strategy we employ does make us safe from introducing binary breaking changes.
|
|
||
| Note that the `RequestParameters` type is not specific to a specific operation, which means it will not have members that correspond to parameters for the operation and so developers will have to depend on documentation to understand the properties the service supports and their names as well as their types. | ||
|
|
||
| *(N.B: We have not closed on which of the two strategies we will employ, and the `RequestParameters` strategy would require some work in Azure Core, fortunately, we can start with the first strategy of just adding overloads with more and more optional parameters and then cut over to the new plan if we are comfortable with it)* | ||
| *(N.B: We have not closed on which of the two strategies we will employ, and the `RequestParameters` strategy would require some work in Azure Core, fortunately, we can start with the first strategy of just adding overloads with more and more optional parameters and then cut over to the new plan if we are comfortable with it.)* |
There was a problem hiding this comment.
Is this still an open question? I hadn't heard yet about RequestParameters, but maybe we're holding it in a back pocket given the ability to move to it later.
There was a problem hiding this comment.
I think the question is still open - the current strategy of adding overloads will work, it will just create a bunch of method overloads. I think there will be a tension here between the following choices:
- Have formal method parameters for every operation parameter. (Works, but can lead to very long method signatures, especially for methods which have many optional parameters).
- Operation specific Options classes (e.g. NewPetOptions) which has a property for each option (method signatures are nicer, but we end up now (possibly) generating a type per operation to hold the options. I do think in practice this may not be the end of the world, especially if it was just optional parameters.)
- A middle ground between 1 and 2 where instead of creating a specific type we use some general RequestParameters thing. This will mean fewer types in the system, but the ergonomics will be worse (similar to how by not generating model classes we make the ergonomics for request bodies worse).
| ``` | ||
|
|
||
| The evolution pattern here is to replace `RequestOptions` with `CancellationToken` in method signature, replace `RequestConent` with a model type that represents the body of the operation and to use `Response<T>` with a model type for operations which return values. | ||
| The evolution pattern here is to replace `RequestOptions` with `CancellationToken` in method signature, replace `RequestContent` with a model type that represents the body of the operation and to use `Response<T>` with a model type for operations which return values. |
There was a problem hiding this comment.
Per discussions I've had with @KrzysztofCwalina lately, it sounds like he is thinking that HLC methods will have no CancellationToken parameter, and LLC methods will take RequestOptions, to prevent the issue around methods that don't take a body. Would it make sense to update this, given that?
There was a problem hiding this comment.
Yes - I can rewrite this based on that observation.
Created for Anne to ask questions in the context of the doc -- follow-up additions will be made based on answers.
Including: