-
-
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
[Elm] Template improvements #661
Conversation
e4fa432 also solves swagger-api/swagger-codegen#8151 |
@laughedelic thanks for the PR 👍 Would removing the last newline in the parent (partial) template help? If you can point me to the line number in the template and show me the actual vs expected output, I can help take a look. cc @trenneman |
As I understand it, the extra newline comes from the child template. So if you have an Here is an example piece of template: openapi-generator/modules/openapi-generator/src/main/resources/elm/modelTypeAlias.mustache Lines 13 to 16 in e4fa432
And the produced result:
One extra newline is added after inlining the Ah, and here is the expected output: apiResponseDecoder : Decoder ApiResponse
apiResponseDecoder =
decode ApiResponse
|> optional "code" (Decode.nullable Decode.int) Nothing
|> optional "type" (Decode.nullable Decode.string) Nothing
|> optional "message" (Decode.nullable Decode.string) Nothing
apiResponseEncoder : ApiResponse -> Encode.Value
apiResponseEncoder model =
Encode.object
[ ( "code", withDefault Encode.null (map Encode.int model.code) )
, ( "type", withDefault Encode.null (map Encode.string model.type_) )
, ( "message", withDefault Encode.null (map Encode.string model.message) )
] (no empty lines in the end and only 2 between the definitions) |
@laughedelic @trenneman I suggest we merge this and deal with the extra newline in a separate PR. |
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've been struggling a lot with newlines as well. It's a PITA with mustache.
A few remarks, but it looks OK other than that (and the remaining newline issue).
@@ -27,6 +27,6 @@ fi | |||
|
|||
# if you've executed sbt assembly previously it will use that instead. | |||
export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties" | |||
ags="generate -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g elm -o samples/client/petstore/elm $@" | |||
ags="generate -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g elm -t modules/openapi-generator/src/main/resources/elm -o samples/client/petstore/elm $@" |
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.
do we need 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.
Yes, I think so as using -t allows us to regenerate the samples without doing another "mvn clean package" after updating the templates.
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.
Yes, I also checked that it's used in other scripts.
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.
It feels a bit weird that it sort of overloads the build, but it makes sense to add it if that's also the case for the other generators.
++ str | ||
++ " to DateOnly" | ||
Result.Err msg -> | ||
Decode.fail msg |
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.
Something like Decode.fail <| "Cannot decode ISO string: " ++ msg
makes clear that it's the decoder that failed
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.
The error message in the new date-extra is
Failed to create a Date from string 'foo': Invalid ISO 8601 format
Do you still want to add "Cannot decode ISO string: "
?
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.
Well the thing is this is a Decoder
error, not a Date
error. But the type already indicates that so I think we can stick with the Decode.fail msg
.
++ str | ||
++ " to DateTime" | ||
Result.Err msg -> | ||
Decode.fail msg |
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.
The same here
Hi! I replied to the review comments and now I'm not sure if there's still anything left to change. |
If no more question/feedback on this PR, I'll merge it into master tomorrow (Monday) |
Nice! |
Hi all, I've filed #879 to improve the code format of the Elm client by removing the EOF from 2 mustache files. Please review to see if it looks good to you. |
#879 has been reviewed and merged into master. Shall we use https://github.com/jfmengels/elm-lint to improve the auto-generated Elm code? (I've no experience using it) |
@wing328 You typically use |
|
Thanks for the suggestion guys. I will see if I can come up with a solution to run |
That would help. However, my preference would be to format it correctly from the start, without any post-processing. |
Totally agree but we found that it's not easy to do so with mustache. For Go, we're trying to use I've created #933 to track the code format improvement |
* Add elm template parameter to the samples testing script * Update elm-date-extra package; fixes OpenAPITools#458 * Update generated elm samples * Use Elm doc comments; remove some unneccessary newlines * Update generated Elm samples * Remove unnecessary parenthesis around non-optional container type
PR checklist
./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\
.master
,4.0.x
. Default:master
.Description of the PR
This PR fixes #458 and improves doc comments and line spacing in the Elm template. (see also swagger-api/swagger-codegen#8152 for the context).
There are still some extra newlines that are hard to get rid of. They appear because the
{{>blah}}
includes bring the last newline in the parent template. I don't know if this can be somehow improved on the Mustache side, but in general it's quite hard to produce some code that immediately complies with the formatting guideline (elm-format).