Skip to content

[Rust][reqwest] various improvements#1890

Merged
wing328 merged 6 commits intoOpenAPITools:masterfrom
bcourtine:rust-reqwest-improve
Feb 1, 2019
Merged

[Rust][reqwest] various improvements#1890
wing328 merged 6 commits intoOpenAPITools:masterfrom
bcourtine:rust-reqwest-improve

Conversation

@bcourtine
Copy link
Contributor

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 and ./bin/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.

/cc @frol @farcaller @bjgill

Description of the PR

This PR contains 3 improvements of the rust client generator with Reqwest library:

@wing328
Copy link
Member

wing328 commented Jan 16, 2019

FYI. The change in body parameter name is due to an enhancement to preserve body parameter name defined in OAS v2 spec

@bcourtine
Copy link
Contributor Author

FYI. The change in body parameter name is due to an enhancement to preserve body parameter name defined in OAS v2 spec

Thanks. I wondered about that when I regenerated the samples. But since the code still compiled, I didn't investigate further.

@wing328
Copy link
Member

wing328 commented Jan 16, 2019

If no question/feedback from anyone, I'll merge this PR tomorrow.

let mut form_params = HashMap::new();
{{#formParams}}
{{#isFile}}
form_params.insert("{{{baseName}}}".to_string(), unimplemented!());
Copy link
Contributor

Choose a reason for hiding this comment

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

I've given this a quick skim. As a thought, when do we want to fail if a user does attempt to create a generator with a file form parameter? This unimplemented would mean that we would fail at run-time when that endpoint was called.

Might it be worth using compile_error! (https://doc.rust-lang.org/std/macro.compile_error.html) so that the user discovers this at compile time instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

compile_error!("File form parameters not supported by the rust generator");, perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ported the Hyper implementation (cf #525). We can switch to compile_error! macro, but we probably should do the same in hyper library templates to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested compile_error! macro and I am questioning…
Since petstore example contains a file form param, with this macro, generated sample code does not compile anymore: the project cannot be used anymore to check syntax error regressions, for example.

So, I suggest keeping unimplemented!.

@bcourtine
Copy link
Contributor Author

Looking for @bjgill comment, I kept unimplemented! macro (cf. discussion), but added support for multipart operations (including file parts).

@bcourtine
Copy link
Contributor Author

@bjgill @wing328 Any news for this PR?

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.

LGTM :shipit:.

Just one minor point for future reference - this PR seems to add a number of blank lines. If you have only one mustache tag per line, you can avoid these sort of extra blank lines. Not a big deal, though - no need to fix for this PR.

@bcourtine
Copy link
Contributor Author

@bjgill Oh, Thanks for the advice. I am not familiar with mustache… I used several tags by line specially because I thought it could reduce blank lines. I will fix that next time I have an evolution to implement.

@wing328 wing328 merged commit 888068d into OpenAPITools:master Feb 1, 2019
@wing328 wing328 changed the title Rust reqwest improve [Rust][reqwest] various improvements Feb 1, 2019
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Resolves OpenAPITools#525 for Rust client generator with reqwest library.

* Use Reqwest "query" method to generate query URL.

* urlencode URL string parameters.

* Generate rust-reqwest client, and verify it compiles and work as intended.

* Map file params (to "&std::path::Path") and support multipart operations (with file params) in Reqwest library.

* Cleanup: template compression to remove unecessary blank lines in generated code.
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.

[Rust] client codegen ignores all fields that are of type "formData"

3 participants