Optional Query Parameter Test #722
Conversation
🦋 Changeset detectedLatest commit: 733eedb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
mcgallan
requested review from
iscai-msft,
lmazuel,
m-nash,
joheredi,
srnagar,
weidongxu-microsoft,
tadelesh and
timotheeguerin
as code owners
| op FromRequired(@query start: string, @query end?: string, @query("api-version") apiVersion: string): void; | ||
|
|
||
| @scenario | ||
| @scenarioDoc(""" |
There was a problem hiding this comment.
we already have tests like these in versioning, what is this adding?
There was a problem hiding this comment.
In fact, the versioning only tests a single optional query parameter and does not test its combination with other query parameters. Additionally, this operation is intended to contrast with the following operation to demonstrate that, regardless of whether the optional parameter is before or after the required parameter, in the generated SDK, the required parameter always precedes the optional parameter.
There was a problem hiding this comment.
I think if we want to test more ordering, we can have http/parameters/optionality with a query section. And the naming shouldn't be fromRequired, because that signifies a growing-up from a required param to a required param + an optional param. Instead we can call this orderingWithRequired or something like that. I also don't see what the difference is between the fromOptional and fromRequired tests in this tsp. They both seem to be testing ordering to me, can you describe how they're different?
There was a problem hiding this comment.
I think if we want to test more ordering, we can have
http/parameters/optionalitywith aquerysection. And the naming shouldn't befromRequired, because that signifies a growing-up from a required param to a required param + an optional param. Instead we can call thisorderingWithRequiredor something like that. I also don't see what the difference is between thefromOptionalandfromRequiredtests in this tsp. They both seem to be testing ordering to me, can you describe how they're different?
Regarding the placement and naming of the operations in this part, I believe your suggestion is a more suitable solution. As for the difference between the two, it might be more intuitive in the generated code from Autorest.Csharp: operation2 represents the expected form from fromoptional, while operation represents the expected form from fromrequired. The difference between the two is that in fromrequired, the required query parameter is placed first, followed by the optional parameter. In fromoptional, it is the opposite, with the required parameter placed after the optional parameter. However, in the generated code, the optional parameter is always placed after the required parameter. This test is meant to illustrate this issue, and the Parameter-TypeSpec project is also aimed at testing this issue.
There was a problem hiding this comment.
@iscai-msft Do you have any suggestions on how to handle this part?
There was a problem hiding this comment.
ordering is the correct way to describe this, since we're playing with the ordering of generated code from service specs
There was a problem hiding this comment.
@iscai-msft The operation names and corresponding description documents have been improved. The optionality folder does not exist under the parameter folder, and creating an optionality folder would conflict with the existing body-optionality folder, so they are temporarily placed in the original folder.
There was a problem hiding this comment.
@iscai-msft You mentioned earlier that the issue with the operation name has been resolved. I also shared my views on the folder naming issue above, but I noticed that you did not respond. Do you have any other suggestions regarding this part?
There was a problem hiding this comment.
sorry, missed this during vacation. Do you see this test as testing ordering more or optionality? Since the file name is optionality, but the operation names are more ordering
There was a problem hiding this comment.
@iscai-msft I think this test focuses more on testing ordering, specifically the impact of optional query parameters on the ordering of query parameters. Since the optional query test case has already appeared in Cadl Ranch, it is not necessary to add this test if it is meant to test optional query parameters. Therefore, renaming this file to query-ordering or something else might be better？
This PR is for migrating Parameter-TypeSpec from autorest.csharp, mainly to test the combination of optional query parameters and their generation behavior in the generated code.