openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[csharp][generichost] Remove discriminator property

Open devhl-labs opened this issue 10 months ago • 2 comments

Please merge this first https://github.com/OpenAPITools/openapi-generator/pull/18408 closes https://github.com/OpenAPITools/openapi-generator/pull/17397

Removes the discriminator because new Whale("whale") is redundant. We can just do new Whale()

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.1.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.

devhl-labs avatar Apr 21 '24 00:04 devhl-labs

Nice improvement, thanks! Just a new minor comments:

  • in the generated code the discriminator constant is now spread across 2 classes. For instance, the constant "BasquePig" exists in Pig.Read() and BasquePig.Write(). Not a big deal because you don't have to maintain generated code.
  • BasquePig.Read() also reads the discriminator, which is kind of duplicate on what Pig.Read() also does. Could be a (minor) performance improvement to skip it.

gerwinjansen avatar May 06 '24 15:05 gerwinjansen

Kept the discriminator property so de/serialization can still work with libraries other than STJ. Hard coded the value of the discriminator property so we can remove it from the constructor. Reworked the manual tests to compare json instead of classes to be more concise. Also added more manual tests to ensure de/serialization works. I think this is ready to go @wing328

devhl-labs avatar May 19 '24 18:05 devhl-labs