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

[elm] Add toString for all types #3983

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Conversation

eriktim
Copy link
Contributor

@eriktim eriktim commented Sep 29, 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 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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.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

Fix #3959 by adding support for any type as a parameter (and exposing a toString for types so each can be serialized)

@auto-labeler
Copy link

auto-labeler bot commented Sep 29, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@eriktim
Copy link
Contributor Author

eriktim commented Sep 29, 2019

@wing328 please do not approve yet. This adds undesired quotes to string types. Need to fix that first.

@eriktim
Copy link
Contributor Author

eriktim commented Sep 29, 2019

@andys8 can you give this PR a try?

@andys8
Copy link
Contributor

andys8 commented Sep 29, 2019

Is this branch published, so it's possible to execute it directly via docker or npm?

@Agraphie
Copy link

Hi there,

I tried the PR but I ran into some issues. I attached a minimal sample yaml. Is this possibly a regression? Thanks for your help!

sample.yml

openapi: "3.0.0"
info:
  version: 1.0.0
  title: message-engine
  license:
    name: MIT
servers:
  - url: http://localhost:9090
paths:
  /message/{messageId}:
    get:
      summary: Get current conversation
      operationId: getConversation
      parameters:
        - name: messageId
          in: path
          required: true
          schema:
            type: string
            format: uuid
      responses:
        "200":
          description: Current conversation
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Message"

components:
  schemas:
    Message:
      type: object
      required:
        - messageType
        - messageId
      discriminator:
        propertyName: messageType
      properties:
        messageId:
          type: string
          format: uuid
        messageType:
          type: string

    MessageStatement:
      allOf:
        - $ref: "#/components/schemas/Message"
        - type: object
          required:
            - text
          properties:
            text:
              type: string



This leads to the following problem in MessageStatement.elm

grafik


MessageStatement.elm

module Data.MessageStatement exposing (MessageStatement, decoder, encode, toString)

import Dict exposing (Dict)
import Json.Decode as Decode exposing (Decoder)
import Json.Decode.Pipeline exposing (optional, required)
import Json.Encode as Encode
import Uuid exposing (Uuid)


type alias MessageStatement =
    { messageId : Uuid
    , text : String
    }


decoder : Decoder MessageStatement
decoder =
    Decode.succeed MessageStatement
        |> required "messageId" Uuid.decoder
        |> required "text" Decode.string


encode : String -> MessageStatement -> Encode.Value
encode tag model =
    Encode.object
        [ ( "messageId", Uuid.encode model.messageId )
        , ( "text", Encode.string model.text )
        , ( "messageType", Encode.string tag )
        ]


toString : MessageStatement -> String
toString =
    Encode.encode 0 << encode




And to the following problem in the request file


grafik


grafik


Default.elm

module Request.Default exposing (getConversation)

import Data.Message as Message exposing (Message)
import Data.Uuid as Uuid exposing (Uuid)
import Dict
import Http
import Json.Decode as Decode
import Url.Builder as Url
import Uuid exposing (Uuid)


getConversation :
    { onSend : Result Http.Error Message -> msg
    , basePath : String
    , messageId : Uuid
    }
    -> Cmd msg
getConversation params =
    Http.request
        { method = "GET"
        , headers = List.filterMap identity []
        , url =
            Url.crossOrigin params.basePath
                [ "message", Uuid.toStringUuid.toString params.messageId ]
                (List.filterMap identity [])
        , body = Http.emptyBody
        , expect = Http.expectJson params.onSend Message.decoder
        , timeout = Just 30000
        , tracker = Nothing
        }

@eriktim
Copy link
Contributor Author

eriktim commented Oct 6, 2019

Is this branch published, so it's possible to execute it directly via docker or npm?

@andys8 Nope it does not get published until it's merged in. If you can build the generator yourself that would be great.

@Agraphie thanks for the detailed report. I'm on it.

@eriktim
Copy link
Contributor Author

eriktim commented Oct 6, 2019

@Agraphie just pushed some fixes. Can you try again?

@Agraphie
Copy link

Agraphie commented Oct 7, 2019

@eriktim looks good to me. I tried it with the minimal example seen above as well as with our project and there aren't any compiler errors. Thanks!

@eriktim
Copy link
Contributor Author

eriktim commented Oct 8, 2019

@wing328 can you have a look at the errors? They seem unrelated to my PR. This should be good to go.

@wing328
Copy link
Member

wing328 commented Oct 9, 2019

Agreed these errors are not related to your PR (some have been fixed in the master already)

@wing328 wing328 merged commit 8cc7080 into OpenAPITools:master Oct 9, 2019
@wing328 wing328 added this to the 4.2.0 milestone Oct 9, 2019
Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

CI failure not related to this PR. (the ruby issue addressed in the master already)

@wing328
Copy link
Member

wing328 commented Oct 31, 2019

@eriktim thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

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.

[Elm] [BUG] Enum paramters not working if not inlined ($ref)
4 participants