Expected structure of 'properties' could be more clear.

[danielballan]

Following up on #24, the expected structure of properties is not clear from the function signature:

https://github.com/NSLS-II/olog/blob/e21d6023b91c38d93a9834066c438aacdcbeb47e/olog/httpx_client.py#L211-L214

We could describe the expected structure in a docstring, or we could "explode" the contents into separate parameters, similar to what we did with tags. Are there good options for a more self-describing function signature?

It looks like property must have a name and that a set of key-value pairs. One way to implement that would be:

def aput_property(name, **attributes)

so that the function accepts arbitrary keyword arguments which are used as the attributes, transformed into the JSON payload like this:

attributes = [{"name": key, "value": value} for key, value in attributes.items()]

That signature imposes two undesirable restrictions, though:

• The attributes' names must all be valid Python identifiers, so we couldn't have attribute names that have spaces in them for instance. (If it's not a valid identifier, it can't be used as a keyword argument.)
• An attribute called name would be disallowed because it would conflict with the required positional argument name. (Python 3.8 solves this problem by adding the concept of positional-only arguments, but I don't think we want to go there.)

Olog-es does not impose either of these restrictions, so neither should we. If we instead do:

def aput_property(name, attributes)

then both of these issues are avoided. The docstring can state that attributes is expected to be an arbitrary dictionary. To be clear, I am suggesting that attributes be structured {<name>: <value>} not [{"name": <name> "value": <value>}] unless olog-es intends to allow two attributes to have clashing names. (Does it, @shroffk?) As shown above, it can rearranged into the list form internally to the function.

The API for aput_properties is more of a problem. No "obvious" API presents itself as far as I can see. We could do:

def aput_properties(named_attributes)

where usage would look like:

aput_properties({"some name": {"key1": value1, "key2": value2}, {"another name": {...}, ...)

It's not exactly self-describing, but a short docstring with an example might suffice. Are there any compelling alternatives? If so, what are their trade-offs?

[ke-zhang-rd]

Clearly, docs is lacking here. No matter what API we choose, I need put more work on docs.

The reason I choose dict as input is tring to make the PUT and GET use/return identical type. I feel that make user easy to understand and use when they refer property. But if the benefit of this identical is very small. I'm ok with @danielballan 's later propoal to drag name out, like

def aput_property(name, attributes)

where usage would look like:

aput_properties({"some name": {"key1": value1, "key2": value2}, {"another name": {...}, ...)

I'm confused with argument of aput_properties. Is it a list of dict or nested dict?

[danielballan]

Could we use this new structure for the return value of GET and PUT as well? {<name>: <value>} seems much more natural as a Python API than {"name": <name>, "value": <value>}. Perhaps we could apply it consistently through our API.

Is it a list of dict or nested dict?

Nested dict.

[ke-zhang-rd]

Then there will be difference between our client GET return with res.json(). I think we need to balance whether to have this new structure in client code level or service return level? attn @shroffk

[shroffk]

So, The current structure makes it very easy to map to the properties object in the service and the csstudio client. Happens automatically by springboot and 2 lines of code in phoebus - I really like thing that work with 2 lines of code.

The proposed structure is possible with would need changes in both the service and the clinet object mapper.

I completely agree that this is not great for the python API. Would it be very difficult / bad for the client to strip out the "name" and "value" labels and expose the properties as Dan recomended? I am open to updating the sevice and phoebus if necessary.

[ke-zhang-rd]

No, client side should be easier. In conclusion, we are gonna change our API to

def aput/get_method(name, attributes):
      ...
      return {<name>: <value>}