Skip to content

Comments

Fix/separate union for S2 message elements#98

Merged
Flix6x merged 10 commits intomainfrom
fix/separate-union-for-S2MessageElements
Mar 25, 2025
Merged

Fix/separate union for S2 message elements#98
Flix6x merged 10 commits intomainfrom
fix/separate-union-for-S2MessageElements

Conversation

@Flix6x
Copy link
Collaborator

@Flix6x Flix6x commented Mar 24, 2025

  • Only classes that have a message_id are typed as s2_python.message.S2Message.
  • All other classes that subclass S2MessageComponent are typed as s2_python.message.S2MessageElement.
  • Adapted unit test to make sure we didn't miss any classes.

Flix6x added 5 commits March 24, 2025 10:26
…t classes

Signed-off-by: F.N. Claessen <felix@seita.nl>
…ent type

Signed-off-by: F.N. Claessen <felix@seita.nl>
…rate-union-for-S2MessageElements

Signed-off-by: F.N. Claessen <felix@seita.nl>

# Conflicts:
#	src/s2python/message.py
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x requested a review from VladIftime March 24, 2025 11:27
@Flix6x Flix6x mentioned this pull request Mar 24, 2025
Flix6x added 2 commits March 24, 2025 12:30
Signed-off-by: F.N. Claessen <felix@seita.nl>
…me about a style issue, ugh

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Copy link
Collaborator Author

Flix6x commented Mar 24, 2025

@VladIftime Could you please help me find out why the pipeline is still failing?

@Flix6x
Copy link
Collaborator Author

Flix6x commented Mar 24, 2025

@jorritn do you think S2MessageElement would be a correct name to describe S2MessageComponents that do not have a message ID (so we therefore do not consider them to be standalone S2Messages)?

Signed-off-by: Vlad Iftime <vladiftime60@gmail.com>
@jorritn
Copy link
Contributor

jorritn commented Mar 24, 2025

@jorritn do you think S2MessageElement would be a correct name to describe S2MessageComponents that do not have a message ID (so we therefore do not consider them to be standalone S2Messages)?

Yes, I think it is. An alternative could be to use S2MessageEntity but an element contains more the aspect that is is supposed to be part of something bigger so let's go for S2MessageElement.

@jorritn
Copy link
Contributor

jorritn commented Mar 24, 2025

One other thing that I would like to point out is that the ReceptionStatus is a S2Message too.

@Flix6x Flix6x merged commit 783533a into main Mar 25, 2025
19 checks passed
@Flix6x Flix6x deleted the fix/separate-union-for-S2MessageElements branch March 25, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants