Rename "value" to "message" and other minor breaking changes #281
Conversation
github-actions
bot
added
part:docs
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We are already using properties for anything that can be trivially calculated, so this looks like a good candidate too to make the interface more consistent. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We prefer to use the term "message" as what's transported in a channel, as "value" is too generic and used in many other contexts. This means changing the code too, so `Selected.value` is now `Selected.message`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is just to make the code extra obvious but it also affects one function signature: `Sender.send()`, so it is a breaking change. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is just for consistency and extra clarity. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Also for extra clarity and consistency. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is to make the code more readable/consistent. Note that `Select` is actually public, but the constructor is not meant to be used by users. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
| @@ -228,16 +228,9 @@ def exception(self) -> Exception | None: | |||
| """ | |||
| return self._exception | |||
|
|
|||
| @property | |||
| def was_stopped(self) -> bool: | |||
There was a problem hiding this comment.
Selected.was_stopped feels a bit ambiguous to me. Might be worth renaming to Selected.receiver_stopped.
There was a problem hiding this comment.
I find it clear enough but I'm also OK to rename it. Will wait a bit in case anyone else wants to chime in.
There was a problem hiding this comment.
I agree the suggested name is more precise. Even though the new documentation will clear up any ambiguity I'd also suggest to rename it
There was a problem hiding this comment.
OK, I was doing the change but then wondered what about exception and message, should they also be receiver_exception and receiver_message? I'm not super convinced about introducing an inconsistency where some stuff in Selected has a receiver_ prefix and some not, but also find a bit overkill to prepend everything with receiver_ as it is pretty clear to me that Selected is wrapping a receiver.
There was a problem hiding this comment.
Other possible options:
Selected->SelectedReceiver(but it might be misleading because it is not really a receiver, just a wrapper)- Remove
was_stoppedand force the user to useexceptioninstead
Not sure why I'm listing them because I think I like them even less than the other options above 😄
There was a problem hiding this comment.
I'm not a fan of was_stopped, but that doesn't mean I can't accept it.
The issues I have with it are that it could mean other things - like the select was stopped/interrupted, etc. Also the past tense was is also confusing. Does that mean we don't know what the present state is?
And it doesn't even qualify that it should be checked within a type narrowed scope of selected_from. It feels like it could be anything.
But of course, nothing wrong with keeping something ambiguous to discourage its use of force people to read the documentation.
There was a problem hiding this comment.
The other fields don't need the receiver_ prefix, because we don't have a was_exception or was_message to begin with.
There was a problem hiding this comment.
I agree that was_ is probably unnecessary, stopped is already a binary state, I think I used it because it read more nicely with if selected.was_stopped than if selected.stopped. Also I think the past tense is still correct. It was stopped when it was used to receive, we don't know if it got somehow restarted since then.
And it doesn't even qualify that it should be checked within a type narrowed scope of
selected_from. It feels like it could be anything.
I didn't get this.
The other fields don't need the
receiver_prefix, because we don't have awas_exceptionorwas_messageto begin with.
I don't get the difference. exception and message are actual objects, not binary states, so it doesn't make sense to have a was_. I thought you suggested the prefix to indicate that it's talking about the receiver, not the Selected object.
In any case, to wrap up, if you are not completely opposed to it and there is no consensus on a better choice, I'd say we just keep the status quo to be able to move forward.
There was a problem hiding this comment.
I will merge this PR, as it is really a separate issue if we want to rename this or not. Feel free to create a issue for it @shsms and we can keep discussing it there.
This PR replaces the term "value" with "message" as "value" is too generic and used in many other contexts.
It also makes
Selected.was_stoppeda property and make some function taking only one obvious argument positional-only, to avoid having the argument name being part of the interface (we actually renamed those arguments too, but that's not included in the release notes as they are not part of the public API any more anyways).We also do some other minor internal renaming and changing of arguments to positional-or-keyword-only.