Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add enum support to rust and skip none option serialization in clients #2244

Merged
merged 2 commits into from
Jun 7, 2019

Conversation

gferon
Copy link
Contributor

@gferon gferon commented Feb 26, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

While generating rust clients, I got blocked by two issues, so here are the fixes.

The first commit adds a serde attribute to skip serialization if an optional field is unset - which means the field will be omitted instead of having "option": null in the request. This is desirable because on the other-side, null is not a valid value.

The second commit adds support for enum generation (the facilities to format name and values was already there in `RustClientCodegen.java) with a change to casing to be more rust-y.

@gferon
Copy link
Contributor Author

gferon commented Feb 26, 2019

Looks like I can't add reviewers myself, so tagging @frol, @farcaller or @bjgill to get a review.

Copy link
Contributor

@bjgill bjgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR ❤️

A couple of minor comments, but nothing serious.

I don't know what our enums used to look like. I don't suppose you could post an example from your testing of what enum variable names now look like once they've been through toEnumVarName?

Edit: Sorry - I was getting confused as to what the camelize function does. Please ignore ^

@gferon
Copy link
Contributor Author

gferon commented Feb 27, 2019

Thanks for the comments @bjgill, I just made fixes for both. I think we'll start using rust generated clients a bit more, so you'll probably hear again from me!

@wing328
Copy link
Member

wing328 commented Mar 4, 2019

@gferon can you please run ./bin/rust-petstore.sh (or the corresponding batch file under bin\windows) to update the Rust petstore samples?

@gferon
Copy link
Contributor Author

gferon commented Mar 4, 2019

@wing328 done, though I'm now trying to understand why the enum won't work with petstore.yaml - is it possible that it's because it's a OpenAPI 2.0 spec?

@wing328
Copy link
Member

wing328 commented Mar 7, 2019

@gferon are you free for a quick chat (IM) to discuss the issue via https://gitter.im (ID: wing328) or other channels today?

@wing328
Copy link
Member

wing328 commented Mar 7, 2019

is it possible that it's because it's a OpenAPI 2.0 spec?

Both OpenAPI 2.0 and 3.0 spec support enum.

Can you elaborate on the issue?

@gferon
Copy link
Contributor Author

gferon commented Apr 24, 2019

@wing328 I'm very sorry about the time it took me to get back to this one. I believe the latest stage is the one I wanted to achieve :) feel free to review again.

Also:
* Skip serializing a field with serde if it's optional and empty
* Fix borrow checker error when using &std::path::Path (should be std::path::PathBuf)
* Add script to generate sample with rust-reqwest
* Regenerate petstore sample for both rust targets
* Remove go code from RDME.md
@wing328 wing328 modified the milestones: 4.0.0, 4.0.1 May 13, 2019
@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
Copy link
Contributor

@bjgill bjgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in re-reviewing. LGTM.

:shipit:

tokenSource := oauth2cfg.TokenSource(createContext(httpClient), &token)
auth := context.WithValue(oauth2.NoContext, sw.ContextOAuth2, tokenSource)
r, err := client.Service.Operation(auth, args)
cargo doc --open
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ the docs updates!

@bjgill
Copy link
Contributor

bjgill commented Jun 7, 2019

I'm just checking that the samples are still up to date, and then I'll merge.

@bjgill bjgill merged commit 3df525e into OpenAPITools:master Jun 7, 2019
@wing328
Copy link
Member

wing328 commented Jun 7, 2019

@bjgill thanks for reviewing and merging the PR 👍

fantavlik added a commit to fantavlik/openapi-generator that referenced this pull request Jun 17, 2019
…to inline-resolver

* 'master' of github.com:OpenAPITools/openapi-generator: (213 commits)
  Idiomatic Rust returns for Error conversions (OpenAPITools#2812)
  Add API timeout handling (OpenAPITools#3078)
  Import inner items for map (OpenAPITools#3123)
  update core team in pom.xml (OpenAPITools#3126)
  [gradle] Document consuming via gradle plugin portal (OpenAPITools#3125)
  Bump up babel-cli version to fix security alert (OpenAPITools#3121)
  [C++] [cpprestsdk] Add examples and test for cpprestsdk (OpenAPITools#3109)
  Add enum support to `rust` and skip none option serialization in clients (OpenAPITools#2244)
  Add/update new core team member: etherealjoy (OpenAPITools#3116)
  Gradle sample on travis (OpenAPITools#3114)
  [typescript-fetch] add bearer token support (OpenAPITools#3097)
  Add Q_DECLARE_METATYPE to the generated models and remove ref in signals (OpenAPITools#3091)
  [Java][okhttp-gson] Update dependencies (OpenAPITools#3103)
  Link query parameter to model object (OpenAPITools#2710)
  scala-play-server: fix enum names for reserved words (OpenAPITools#3080)
  Add @Sunn to openapi generator core team (OpenAPITools#3105)
  fix NPE in go generator (OpenAPITools#3104)
  scala-play-server: fix API doc url (OpenAPITools#3096)
  [maven-plugin] fix strictSpec parameter without alias (OpenAPITools#3095)
  Ruby: Avoid double escaping path items (OpenAPITools#3093)
  ...

# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/InlineModelResolverTest.java
@wing328
Copy link
Member

wing328 commented Jun 20, 2019

Thanks again for the PR, which has been included in the v4.0.2 release: https://twitter.com/oas_generator/status/1141610197766426626

@gferon gferon mentioned this pull request Sep 16, 2019
4 tasks
bjgill pushed a commit that referenced this pull request Oct 6, 2019
* Add support for the `discriminator` feature of OpenAPI 3, and implement it with `enum` in Rust
* Add all missing explicit `dyn` to trait types to remove warnings
* Add missing re-export for properties that are enum (was missing from #2244).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants