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

[OIL] Templatization of PS Remove-Xen* cmdlets. Refactoring of HTTP_actions.mustache #5554

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

kc284
Copy link
Contributor

@kc284 kc284 commented Apr 11, 2024

First installment of PS templatization.

@kc284 kc284 marked this pull request as draft April 11, 2024 10:08
@kc284 kc284 force-pushed the private/konstantin1/sdk-ps branch from 24cc4a7 to 4e8200b Compare April 11, 2024 20:02
@kc284 kc284 marked this pull request as ready for review April 11, 2024 20:27
@duobei duobei self-requested a review April 18, 2024 08:27
Comment on lines 100 to 105
let cmdlets =
classes |> List.filter generated |> List.map gen_cmdlets |> List.concat
in
List.iter (fun x -> write_file x.filename x.content) cmdlets
List.iter (fun x -> write_file x.filename x.content) cmdlets ;

classes |> List.filter generated |> List.iter gen_destructor
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let cmdlets =
classes |> List.filter generated |> List.map gen_cmdlets |> List.concat
in
List.iter (fun x -> write_file x.filename x.content) cmdlets
List.iter (fun x -> write_file x.filename x.content) cmdlets ;
classes |> List.filter generated |> List.iter gen_destructor
let classes_filterd = List.filter generated classes in
let cmdlets =
classes_filterd |> List.map gen_cmdlets |> List.concat
in
List.iter (fun x -> write_file x.filename x.content) cmdlets ;
List.iter gen_destructor classes_filterd

Copy link
Member

Choose a reason for hiding this comment

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

List.concat_map can be used to generate cmdlets as well

Comment on lines 168 to +197
`O
[
("licence", `String Licence.bsd_two_clause)
; ("http_actions", `A (List.map action_json filtered_actions))
( "http_actions"
, `A
(List.map
(fun (name, (meth, uri, _, sdkargs, _, _)) ->
`O
[
("name", `String name)
; ("isPut", `Bool (meth == Put))
; ("isGet", `Bool (meth == Get))
; ("uri", `String uri)
; ( "args"
, `A
(List.map
(fun x ->
`O
[
("arg_decl", `String (decl_of_sdkarg x))
; ("arg_use", `String (use_of_sdkarg x))
]
)
sdkargs
)
)
]
)
filtered_actions
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe below is little better:

let of_arg arg =
    `O
      [
        ("arg_decl", `String (decl_of_sdkarg arg))
      ; ("arg_use", `String (use_of_sdkarg arg))
      ]
  let of_action (name, (meth, uri, _, sdkargs, _, _)) =
    `O
      [
        ("name", `String name)
      ; ("isPut", `Bool (meth == Put))
      ; ("isGet", `Bool (meth == Get))
      ; ("uri", `String uri)
      ; ("args", `A (List.map of_arg sdkargs))
      ]
   in
  `O [( "http_actions", `A (List.map of_action filtered_actions))]

@kc284 kc284 self-assigned this Apr 25, 2024
@danilo-delbusso danilo-delbusso self-requested a review May 7, 2024 12:53
@kc284 kc284 force-pushed the private/konstantin1/sdk-ps branch from 4e8200b to 3721583 Compare June 12, 2024 15:24
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #5554     +/-   ##
========================================
- Coverage    51.4%   44.8%   -6.6%     
========================================
  Files          13      16      +3     
  Lines        1928    2210    +282     
========================================
+ Hits          991     992      +1     
- Misses        937    1218    +281     

see 4 files with indirect coverage changes

Flag Coverage Δ
python2.7 45.5% <ø> (?)
python3.11 51.4% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@danilo-delbusso danilo-delbusso left a comment

Choose a reason for hiding this comment

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

Signed-off-by: Konstantina Chremmou <Konstantina.Chremmou@cloud.com>
@kc284 kc284 force-pushed the private/konstantin1/sdk-ps branch from 3721583 to 694da51 Compare July 12, 2024 12:12
@kc284 kc284 requested a review from xueqingz July 12, 2024 12:28
@@ -21,11 +46,11 @@ namespace XenAPI
}

{{#http_actions}}
public static void {{name}}(HTTP.{{delegate_type}} {{delegate_name}}, HTTP.FuncBool cancellingDelegate, int timeout_ms,
string hostname, IWebProxy proxy, string path, {{{sdkargs_decl}}})
public static void {{name}}(HTTP.{{#isPut}}UpdateProgressDelegate progressDelegate{{/isPut}}{{#isGet}}DataCopiedDelegate dataCopiedDelegate{{/isGet}}, HTTP.FuncBool cancellingDelegate, int timeout_ms,
Copy link
Contributor

Choose a reason for hiding this comment

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

If there just two possible cases, "Put" or "Get", I think 1 var "isPut" is enough, like

HTTP.{{#isPut}}UpdateProgressDelegate progressDelegate{{/isPut}}{{^isPut}}DataCopiedDelegate dataCopiedDelegate{{/isPut}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, yes, we only have Put and Get methods, but I used two separate variables isPut and isGet in case it is expanded in future to include other types of methods like Post etc.

{{#async}}
[OutputType(typeof(XenAPI.Task))]
{{/async}}
[OutputType(typeof(void))]
Copy link
Contributor

@xueqingz xueqingz Jul 15, 2024

Choose a reason for hiding this comment

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

Do we miss {{^async}} here for "[OutputType(typeof(void))] "?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, when the call has an async version the Task will be returned only if the switch -PassThru is specified. If the user does not use the switch, the return type is void, so the OutputType attribute needs to describe both cases. However, one thing I've just noticed is that we don't need the first line [OutputType(typeof({{type}}))], because the object is deleted so it cannot be returned. This looks like an issue in the old generation code and I just copied it over without thinking. I'll mark the PR as needs updating and have a better look, thanks for bringing it up 👍

Signed-off-by: Konstantina Chremmou <Konstantina.Chremmou@cloud.com>

# Conflicts:
#	ocaml/sdk-gen/powershell/gen_powershell_binding.ml
@kc284 kc284 merged commit bd222d0 into xapi-project:master Jul 16, 2024
15 checks passed
@kc284 kc284 deleted the private/konstantin1/sdk-ps branch July 16, 2024 12:41
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.

6 participants