Skip to content
This repository was archived by the owner on Feb 19, 2025. It is now read-only.

Conversation

@kc284
Copy link

@kc284 kc284 commented Oct 2, 2020

CA-345342: Made the query parameters of the http actions optional using as default value the type default.

Note that some of them are not optional, however, the current definition does not allow for distinguishing them from the mandatory ones, and as a consequence it is easier to mark them all as optional than provide method overloads manually.
Also, exposed manually in the SDK http actions removed from the API.
Use a mustache template to generate the HTTP actions.

@robhoes
Copy link
Member

robhoes commented Oct 2, 2020

using as default value the type default

Can you actually omit the parameters from the HTTP action, rather than filling in arbitrary defaults? I know that the C# bindings do this for RPCs as well, and it was hard to do it differently. However, in this case, it seems to be avoidable. I read something about adding the [Optional] attribute to the parameters rather than specifying a default, and then doing a null check in the function body.

| String_query_arg s
| Int64_query_arg s
| Bool_query_arg s -> "\"" ^ s ^ "\", " ^ (escaped s) (* "s", s *)
| Bool_query_arg s -> sprintf "\"%s\", %s" s (escaped s)
Copy link
Member

@psafont psafont Oct 2, 2020

Choose a reason for hiding this comment

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

Suggested change
| Bool_query_arg s -> sprintf "\"%s\", %s" s (escaped s)
| Bool_query_arg s -> sprintf {|"%s", %s|} s (escaped s)

It can be written without escaping the quotation marks, making it more readable.

@kc284
Copy link
Author

kc284 commented Oct 2, 2020

The default values provided are not arbitrary, they are the defaults for the specific type. If one replaces the explicit declaration with the [Optional] attribute, the compiler will still apply the type defaults, which means false and 0 for bool and long respectively, not null. The SDK does omit the null and false args when constructing the URL query, but this leaves us indeed with the 0 for the long args. Perhaps what we should do here is change the bool and long args to nullable bool? and long?, so they can all accept null as the default.

@robhoes
Copy link
Member

robhoes commented Oct 2, 2020

The defaults are well-defined for C#, but unrelated to the defaults applies by xapi. We may well define one of the Boolean args to have a default of true. If the SDK then omits false values, even if explicitly set, then you get the wrong behaviour. Having null as a default for all argument types, and then omit null args from the request, will avoid this.

… nullable.

Note that some of them are not optional, however, the current definition does not
allow for distinguishing them from the mandatory ones, and as a consequence it is
easier to mark them all as optional than provide method overloads manually.
Also:
- Fixed issue where the parameters set to false were ignored.
- Exposed manually in the SDK http actions removed from the API.

Signed-off-by: Konstantina Chremmou <[email protected]>
Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

I think this looks good now.

@kc284 kc284 merged commit e7615d2 into xapi-project:master Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants