Small inconcistency in optional fields

[llucax]

What happened?

In https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1alpha8/microgrid/sensors/sensors.proto#L92-L97:

  // Optional vendor-provided diagnostic code for the error, for vendor-specific
  // insights or more granular diagnostics.
  optional string vendor_diagnostic_code = 2;

  // Optional human-readable message providing additional context.
  string message = 3;

Both fields are optional but one has the explicit optional marker and the other one doesn't.

This makes parsing inconsistent too, as one field must be checked via HasField() and the other one should be checked comparing to the empty string to know if it is set or not.

What did you expect instead?

Both fields have optional.

Affected version(s)

v0.8.0

Affected part(s)

The protocol buffer definition files (part:protobuf)

Extra information

I didn't check if we have other instances of inconsistencies like this one. Maybe it is worth checking.

[tiyash-basu-frequenz]

To me it looks like the documentation for the field string message is incorrect. I suppose what was meant was that the error message can be an empty string.

I will fix it.

[llucax]

It still feels weird to me that one field represents and empty value as an empty string and the other as the whole field being missing.

Why do you think the difference makes sense in here? Why not just having both optional, or having both use the empty string as the lack of a meaningful value?

In practical terms, I think using the empty string as the lack of a value makes sense, because it is much less annoying to use (no need to call HasField()) and I doubt any vendor will use an empty string as a meaningful diagnostic code, so distinguishing between an empty string and the the absence of the field probably doesn't bring any value.

[tiyash-basu-frequenz]

It still feels weird to me that one field represents and empty value as an empty string and the other as the whole field being missing.
Why do you think the difference makes sense in here? Why not just having both optional, or having both use the empty string as the lack of a meaningful value?

Both are valid points.

I am thinking if whether vendor_diagnostic_code makes sense as an optional field or not. Hardware units almost always provide error details, in which case making this optional is probably incorrect. The detail might just say the equivalent of "generic error" though. I guess that's why it has to be optional.

In fact, message is inherently optional (in our concept at least) - as in, error codes should almost always be enough to convey the error, but for the odd case a detailed message may be needed, if it is possible to formulate one. E.g., the hardware might say "generic error" and we add any extra information we find.

Let me validate my thoughts though. I will get back here.

[llucax]

Just for as a final clarification, I think the key question is if an empty string is a meaningful value or not for users. If a user needs to treat an empty string as the lack of a value, then making the field optional only makes things more complicated and confusing. In Python for when "" is semantically equivalent to not msg.HasField(), then I translate "" to None (and the lack fo the field too, of course).

If an empty string has a meaningful value (and the field is optional), then of course "" stays as an "" in Python and only the lack of the field translates to None.

Besides what we end up doing, I think it would be valuable to document in the API if an empty value has a meaning besides field missing/default for fields said to be optional, as in protobuf the empty string has a special treatment as being the default.