-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[crystal] Fix some issues in crystal client templates #10629
Conversation
@@ -245,18 +146,19 @@ module {{moduleName}} | |||
when :pipes | |||
param.join("|") | |||
when :multi | |||
# TODO: Need to fix this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about raise
an error saying multi
is not supported in the client?
when {{{value}}} | ||
{{{name}}} | ||
{{/enumVars}} {{/allowableValues}} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to allow trailing spaces:
{{#allowableValues}}
{{#enumVars}}
when {{{value}}}
{{{name}}}
{{/enumVars}}
{{/allowableValues}}
Raises on unsupported feature clean up extra new lines in partial_model_enum_class.mustache
length is undefined for String or Array
# return the array directly as typhoeus will handle it as expected | ||
param | ||
# TODO: Need to fix this | ||
raise "mult is not supported yet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo mult
=> multi
I ran the crystal petstore integration tests (samples/client/petstore/crystal/pom.xml) locally but got
Looks like there's a breaking change in initializing the model (pet in this case)? Can we remove the breaking change for the time being so that rest of the fixes/enhancements can be merged into master first? |
The spec could also have a File model
@@ -432,7 +432,7 @@ module Petstore | |||
# uploads an image | |||
# @param pet_id [Int64] ID of pet to update | |||
# @return [ApiResponse] | |||
def upload_file(pet_id : Int64, additional_metadata : String?, file : File?) | |||
def upload_file(pet_id : Int64, additional_metadata : String? = nil, file : File? = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyangle Is this (default to nil) also breaking change when it comes to calling the operations? If yes, please revert the change for the time being?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it
single quotes are used for single char in crystal
@wing328 I have reverted the breaking changes and also added file upload support and other bug fixes |
I have no idea how to fix this issue:
In crystal, a method can specify default values for the last parameters from here |
I reran the tests and now I got
Has there been any breaking changes to the model properties? |
Btw, here is how you can run the test locally with a petstore server running in your machine: |
All local tests passed:
|
@wing328 Fixed some issues found when using crystal generator to generate client library for google drive v3 api
tests failed with below errors:
PR checklist
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*
.For Windows users, please run the script in Git BASH.
master
(5.3.0),6.0.x