Optional Query Parameter Test #722
Conversation
🦋 Changeset detectedLatest commit: 59f7d42 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.
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？
There was a problem hiding this comment.
i think this could also be included with the optional query test case, since this tests ordering with optionality. What are your thoughts on adding these 2 tests next to existing tests about optional parameters. Additionally, I'm not sure how important it is that they are "query" parameters, this seems to be testing ordering with optionality
There was a problem hiding this comment.
@iscai-msft I made some modifications based on your suggestions. How does this version of the test look now?
There was a problem hiding this comment.
@iscai-msft I have optimized the query parameter to a general parameter to ensure the test focuses on the optionality ordering of the parameters. I have also moved it to the parameter/body-optionality section. Additionally, I have made changes to the code based on the subsequent mockapi upgrades for the PR. Please help review.
There was a problem hiding this comment.
I don't think this spec belong in cadl ranch, this is just a test that belong in the csharp emitters imo.
It no effect on teh communication it is just a signature verification which cadl-ranch isn't able to validate.
And other emitters might not want the same rules as well
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.