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

CP-47354, CP-47355: add mustache template for messages and generate GO methods code #5591

Merged
merged 6 commits into from
May 9, 2024

Conversation

duobei
Copy link
Contributor

@duobei duobei commented Apr 25, 2024

  1. Add mustache template for xapi data module class messages
  2. Generate messages functions Golang code for all classes

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

Has the OCaml code gone trough ocamlformat and the Go code through gofmt?

ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_binding.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
@duobei
Copy link
Contributor Author

duobei commented Apr 25, 2024

Has the OCaml code gone trough ocamlformat and the Go code through gofmt?

Both have gone through.

@minglumlu
Copy link
Member

I would suggest a basic structure like:

module Convert = struct
  type params = {func_name_suffix: string; value_ty: string}

  type params_of_enum_item =
    { value: string
    ; name: string
    }

  type parms_of_enum =
    { func_name_suffix: string
    ; value_ty: string
    ; items: params_of_enum_item
    }

  type params_of_map =
    { func_name_suffix: string
    ; value_ty: string
    ; key_ty: string
    ; val_ty: string
    }

  ...

  type convert_params =
    | Simple of params
    | Int of params
    | Enum of params_of_enum
    | Map of param_of_map

  let template_of_convert = function
    | Default of _ ->
        "ConvertSimpleType.mustache"
    | Int of _ ->
        "ConvertInt.mustache"
    | Float of _ ->
        "ConvertFloat.mustache"
    | ...


  let to_json =function
    | Simple params ->
    | Int params ->
        json_of_params params
    | ...

  let of_ty = function
    match ty with
    | SecretString | String ->
        Simple {func_name_suffix: "String"; value_ty: "string"}
    | Int ->
        Int {func_name_suffix: "Int"; value_ty: "int"}
    | Bool ->
        Default {func_name_suffix: "Bool"; value_ty: "bool"}
    | DateTime ->
        ...
    | Enum (name, kv) as ty ->
        ...
    | ...

end

all_types
|> List.map Convert.of_ty
|> List.fold_left (fun acc params ->
    let template = Convert.template_of_convert params in
    (* merge by template*)
)
|> List.iter(fun template, params_list ->
    render_template template params_list
)

@minglumlu
Copy link
Member

This PR is too big. Hopefully it could be split into at least 3 smaller PRs.

  1. The go code should be in one PR separately.
  2. render messages
  3. render converts
  4. others?

@duobei
Copy link
Contributor Author

duobei commented Apr 26, 2024

This PR is too big. Hopefully it could be split into at least 3 smaller PRs.

  1. The go code should be in one PR separately.
  2. render messages
  3. render converts
  4. others?

Sorry, I should make the PR reviewed easily. I'll split it to 3 smaller PRs today.

@duobei duobei force-pushed the private/CP-48667 branch 2 times, most recently from 51e99f1 to ae6eedd Compare April 28, 2024 11:49
@minglumlu
Copy link
Member

A general comment on the template is if we consider a template as a function, then its input (and its output) should be defined exactly. This would be useful to make sure the template variables are not missing and the unnecessary ones can be cleaned up.
It's also possible to use ppx to auto-gen the mustache JSON from records. This would be helpful to ensure the correctness of the template rendering.
The above could be an improvement in the future.

@xueqingz
Copy link
Contributor

xueqingz commented May 6, 2024

…lass messages

Signed-off-by: Luca Zhang <feiya.zhang@cloud.com>
duobei added 2 commits May 6, 2024 20:13
Signed-off-by: Luca Zhang <feiya.zhang@cloud.com>
…h_enums`

Signed-off-by: Luca Zhang <feiya.zhang@cloud.com>
@xueqingz
Copy link
Contributor

xueqingz commented May 7, 2024

Checked the generated Go source files' methods based on new commit, looks good to me. Thanks.

Signed-off-by: Luca Zhang <feiya.zhang@cloud.com>
@psafont
Copy link
Member

psafont commented May 8, 2024

Ming had done an in-depth review, so I believe this looks OK from a toolstack perspective, it needs a review from the maintainers of the SDK, interfaces

@xueqingz xueqingz self-requested a review May 9, 2024 01:28
Copy link
Contributor

@xueqingz xueqingz left a comment

Choose a reason for hiding this comment

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

Checking the generated Go source files with golangci-lint:

root@3eea0d3e0e5d:/app/go/goSDK# golangci-lint run --config=/app/.golangci.yml
WARN The linter 'execinquery' is deprecated (since v1.58.0) due to: The repository of the linter has been archived by the owner.  
WARN The linter 'gomnd' is deprecated (since v1.58.0) due to: The linter has been renamed. Replaced by mnd. 
0 issues.

Test with current xenserver-samples test. No special issues.

ocaml/sdk-gen/go/templates/Methods.mustache Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/test_data/session_method.go Outdated Show resolved Hide resolved
@duobei duobei merged commit ba0efb4 into xapi-project:feature/go_sdk May 9, 2024
13 checks passed
Copy link

github-actions bot commented May 9, 2024

pytype_reporter extracted 50 problem reports from pytype output

.

You can check the results of the job here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants