[csharp][generichost] Fixed parameter ordering

devhl-labs commented 1 month ago

The models properties were recently fixed, but operations parameters were missed. We sort by name initially just for a clean presentation, but it has to be ordered such that nullable, not required, or has default value parameters are at the end. The logic used now is the same as the model properties.

PR checklist

[x] Read the contribution guidelines.
[x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
[x] Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH) Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
[x] File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
[ ] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

wing328 commented 1 month ago

thanks for the PR, shouldn't the parameter order base on the order in the openapi spec?

devhl-labs commented 1 month ago

No, it would not compile. Any parameter with a default value must go at the end.

wing328 commented 1 month ago

if you look at this change: https://github.com/OpenAPITools/openapi-generator/pull/18823/files#diff-47079d14a423b64dc3f49b67fde4038768e84ea028425341340ce2f1f6943949R162, password and username are swapped even though both do not have default value.

for parameters without default value, can we keep the same order as stated in the spec? otherwise this PR becomes a breaking change for many as the function signature is changed.

devhl-labs commented 1 month ago

It should be alphabetical. This is the same logic used by the models which was also changed recently.

devhl-labs commented 1 month ago

Plus the ordering from the spec wasnt what was used before anyway. It was by data type which makes no sense.

Should this change target the 8.0 branch? Or better yet, how about I put this change behind a switch which defaults to false until 8.0?

wing328 commented 1 month ago

It should be alphabetical

is there an issue/request from the user to mandate parameters sorted in alphabetical order? I don't recall seeing any related request (parameter ordering to be alphabetical) for other generators (e.g. java, ruby, etc)

this is not to say it's not a valid request but I would like to understand the use case a bit more but making such decision as the parameter ordering impacts a lot of users.

if it's simply to put certain parameters (e.g. those with default values) at the back of the parameter list in the constructor, then I think I can come up with a solution while maintaining the original order provided in the spec

devhl-labs commented 1 month ago

This pr started when Szabolcs Kelemen posted a spec in json that does not work for generichost. The issue is that nullable, not required, or has default must go to the end, because we set them equal to null. Any value that we give a default to must go to the end. The alphabetical addition is to match what we do with model constructors. Previously, we sorted by data type which does not make much sense. I understand the concern, but there is a fallback. I don't want us to be shackled to old ways just for convenience sake. I must be able to make breaking changes for the health of the whole, which should be understandable especially when this library is still marked as experimental and subject to change. Note that I think we can remove the experimental tag very soon, but it does not remove the fact that breaking changes should be allowed with major version changes, especially when a fallback is provided.

wing328 commented 1 month ago

This pr started when Szabolcs Kelemen posted a spec in json that does not work for generichost.

can I take a look please and give it a try?

I must be able to make breaking changes for the health of the whole

we definitely allow breaking changes. i'm not against these at all.

just wanna share I may have a way to address the issue (not saying my way should be preferred or better)

have you confirmed with Szabolcs Kelemen that he's ok with parameters sorted alphabetically or he actually prefers to keep the original order in the spec if possible?

I agree with you one way to address the issue it to sort the parameters alphabetically but I just don't think that's what the users (at least the majority) want in the output.

The alphabetical addition is to match what we do with model constructors.

I do remember there's a request about keeping the properties order a while ago.

Previously, we sorted by data type which does not make much sense.

I agreed it shouldn't be sorted by data type

looking at https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/csharp/restsharp/net7/Petstore/src/Org.OpenAPITools/Api/FakeApi.cs#L3482 https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/csharp/generichost/net8/Petstore/src/Org.OpenAPITools/Api/FakeApi.cs#L409

it doesn't occur to me these are sorted by data type. do you mind providing an example?

Should this change target the 8.0 branch? Or better yet, how about I put this change behind a switch which defaults to false until 8.0?

it's perfectly fine to target the current master branch as you've added an option to let users choose (thanks for the option)

devhl-labs commented 1 month ago

I thought you were arguing for no change at all, but you just want spec ordering. Sorry for being so argumentative. It's not currently using either, as you can see here. To make it spec ordering, we would just remove the initial sort. Whether we use spec ordering or alphabetical, we should do the same for model constructors.

wing328 commented 1 month ago

Sorry for being so argumentative

No worry bro. We all try to do good things. My answer wasn't very clear as I was too busy these days and didn't have time to elaborate

OpenAPITools / openapi-generator

[csharp][generichost] Fixed parameter ordering #18823

PR checklist