Remove unnecessary optional keywords and add documentation to `UpdateGridpoolOrderRequest` about frequenz-api-electricity-trading HOT 11 CLOSED

Haven't I done this? Whats on your mind beyond of what I added to the documentation?

Oh right, you did that already. I'd maybe extend it to add a suggestion what happens when it is left empty, wherever it is possible.

from frequenz-api-electricity-trading.

Why is the optional keyword not removed for the stop price?

That was a mistake. Its fixed now.

from frequenz-api-electricity-trading.

The changes have been merged in #21.
@camille-bouvy-frequenz can we close this issue?

from frequenz-api-electricity-trading.

The recommendation makes sense.

I'd also recommend adding that these fields are optional to the field documentations, so that users get informed of it when they use these fields in their IDEs/code-editors.

from frequenz-api-electricity-trading.

fields are optional to the field documentations

Haven't I done this? Whats on your mind beyond of what I added to the documentation?

from frequenz-api-electricity-trading.

Why is the optional keyword not removed for the stop price?

optional frequenz.api.common.v1.market.Price stop_price = 8;

And what is the 'sane default' of delivery_period and delivery_area?

// Optional filter for delivery period.
optional frequenz.api.common.v1.grid.DeliveryPeriod delivery_period = 3;

// Optional filter for delivery area.
 optional frequenz.api.common.v1.grid.DeliveryArea delivery_area = 4;

Update: These should not have the optional keyword (as their sane defaults are None). Will be updated.

from frequenz-api-electricity-trading.

And what is the 'sane default' of delivery_period and delivery_area?

There is none. If they are not specified they will be rejected. They have to be explicitly given. They are also not optional hence I'm not sure where you copied the lines above from. So far these are not optional.

It seems this does not belong to this issue as they are about filtering and not about creating or updating things.

from frequenz-api-electricity-trading.

I'm not sure where you copied the lines above from

From the GridpoolOrderFilter here, and same applies to PublicTradeFilter.
But yes I recon they should not have the optional keyword, I'll change it with the rest.

from frequenz-api-electricity-trading.

But yes I recon they should not have the optional keyword, I'll change it with the rest.

No this is part of the filter. Both DeliveryPeriod and DeliveryArea are simple messages from my understanding and hence they can have the optional keyword in front of them. To me that looks correct. It means they do not need to be specified and if they are not specified there is no default! These are however used in combination with the field masks.

The default for an empty message is an empty message. That means they don't have a "default" as with an enum. The optional keyword enables field presence tracking. That means we can differentiate between a user explicitly given an empty message and saying I want this message to be reset. There is three states:

1. Setting the message but not including in the field mask
2. Setting a message and including in the field mask
3. Not setting a message but including it in the field mask

Each has different repurcushions but especially for enums because lets say we'd want to remove an enum value and NOT set it to its default. That would be impossible without the optional keyword. Now you are right for messages we might be able to remove it as we don't necessarily need the field presence tracking on the other handside it doesn't hurt knowing when something has been explicitly set and when not because an empty message might not equal an empty message.

More can be found here.

from frequenz-api-electricity-trading.

if they are not specified there is no default

I thought their default would precisely be None (since they are composite types). And thus the reason why the optional keyword was not needed here.

They can never be a "default" as with an enum

My understanding was that the 'sane default' for enum is 0 or UNSPECIFIED as we have it. So that for these we needed to keep the optional keyword when needed.

from frequenz-api-electricity-trading.

You are right it can be removed. The optional keyword is redundant in this case as explicit field presence can be tracked wether the optional keyword is present or not for simple message types.

from frequenz-api-electricity-trading.