-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add S2 Message Union type #37
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Victor Garcia Reolid <[email protected]>
@victorgarcia98 Apologies for a late response. I do not work at TNO anymore and it took some time to get settled in my new situation again. Reviewing now! |
@@ -0,0 +1,12 @@ | |||
import inspect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a version of this file in generate_s2_message_type_to_class.py
to generate a mapping from message_type
literals into the appropriate class. You are using it to generate a list of all message types into an alternative S2Message
type which only contains messages.
I think we can combine the 2. My proposal would be to move TYPE_TO_MESSAGE_CLASS
to the new src/s2python/message.py
file and generate this entire file with a single script. The result would be that we generate both the TYPE_TO_MESSAGE_CLASS
constant as well as the S2Message
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that this suggestion is purely a refactoring request?
Since I'll be taking over this PR on behalf of @victorgarcia98, and I'm still a little uncertain as to increase the scope of this PR's code changes, would it be okay to tackle this refactoring in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not an issue at all, lets do it in a seperate PR. It is an idea purely for refactoring/code deduplication. If you are busy, I can submit the PR for review to you.
SessionRequest | ||
) | ||
|
||
S2Message = Union[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name S2Message
is already used here:
class S2Message(Generic[C], ValidateValuesMixin[C], BaseModel): |
My proposal is to change the original usage from S2Message
to something like S2MessageComponent
as it is more descriptive and keep S2Message
as you introduce it here. Especially since the members of this Union type are all the messages and not parts of a message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 😄 Will keep this open so you can optionally resolve it. (I usually keep the conversations open until the commit is made to keep an overview, but feel free to use it however you want of course)
@@ -214,7 +214,7 @@ class Config: | |||
..., description="ID of this instruction (as provided by the CEM) " | |||
) | |||
message_id: ID = Field(..., description="ID of this message") | |||
message_type: str = Field("InstructionStatusUpdate", const=True) | |||
message_type: Literal["InstructionStatusUpdate"] = Field(default="InstructionStatusUpdate") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues with moving from a pydantic const to the Literal
type. However, datamodel-codegen does not support to use it for pydantic v1 output. Prioritising updating to pydantic v2 so it is automatically generated. #10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is ready that has taken care of generating Literals instead of constants by moving to pydantic v2: #40
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HomeAssistant is not yet compatible with Pydantic 2. We are tracking home-assistant/core#99218 to see when it is, but until then I'm not sure of the implications with regards to compatibility if the s2-python
package would already migrate to Pydantic 2. Is the use of a Literal
problematic at this point, or do you only regard it as a less preferred option over moving to Pydantic first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @Flix6x ! gen_s2.py is the output of the datamodel-codegen code generator. As such, we can move to literals but in its current shape it would be modifying a generated code file manually or as a post-processing step.
Perhaps we should split the discussion. We could discuss 'Why and when to move to pydantic v2' here: #10
Then we can keep the discussion in this comment thread to how to close this PR. Moving to pydantic v2 solves this automatically as datamodel-codegen uses the Literal
type in its output for pydantic-v2
output. So I was expecting that moving to pydantic v2 would solve this issue without any extra work. However, if we want to move to Literal
without moving to pydantic v2, I would like to propose an alternative. Manually altering generated files is error prone. We could move the Literal
lines from the generated code that are introduced by this PR to the non-generated classes, essentially overwriting the fields as we have done with ID's. The referenced line of this comment refers to InstructionStatusUpdate
. An example on where to move it (see the last line):
@catch_and_convert_exceptions
class InstructionStatusUpdate(
GenInstructionStatusUpdate, S2Message["InstructionStatusUpdate"]
):
class Config(GenInstructionStatusUpdate.Config):
validate_assignment = True
message_id: uuid.UUID = GenInstructionStatusUpdate.__fields__["message_id"].field_info # type: ignore[assignment]
instruction_id: uuid.UUID = GenInstructionStatusUpdate.__fields__[
"instruction_id"
].field_info # type: ignore[assignment]
message_type: Literal["InstructionStatusUpdate"] = Field(default="InstructionStatusUpdate") # <-- Line which is added
What do you think of this idea? In case you like it, perhaps its a good idea to first test with a small case to see if it has the intended effect.
Recently, I had the need to wrap the S2 messages in an envelope to simulate accelerated interactions between a CEM and a RM.
Nonetheless, for this to work, the
message_type
had to be aLiteral
. This PR refactors the code to use Literals instead of regular fields.Example usage: