Skip to content

Conversation

@seba-aln
Copy link

No description provided.

@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from ce4d004 to 1109fadCompareFebruary 9, 2023 10:31
@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from 64eb34a to 6c2e89aCompareFebruary 27, 2023 08:44
Sebastian Molenda added 2 commits February 27, 2023 10:04
@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from 6c2e89a to 00e1b16CompareFebruary 27, 2023 09:05
cleanup + add acceptance test action
@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from 00e1b16 to 4b4776eCompareFebruary 27, 2023 16:03
params={'max': int(self._count)}
params={
'max': int(self._count),
'include_type': 'true'ifself._include_message_typeelse'false',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'include_type': str(self._include_message_type).lower()

params={
'max': int(self._count),
'include_type': 'true'ifself._include_message_typeelse'false',
'include_message_type': 'true'ifself._include_message_typeelse'false',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'include_type': str(self._include_message_type).lower()

ifself._include_message_typeisnotNone:
params['include_message_type'] ="true"ifself._include_message_typeelse"false"
ifself._include_space_idisnotNone:
params['include_space_id'] ="true"ifself._include_space_idelse"false"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params['include_space_id'] = str(self. _include_space_id).lower()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space_id can be null and in this case the parameter should not be added, that's why there's if statement

@@ -0,0 +1,27 @@
classPNMessageType:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class look like a good example of enum class:
https://docs.python.org/3/library/enum.html

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well Enum is a closed set of values - this is more like a wrapper around two independent message types:

  • One is user-defined and can be any string (with some constrains on server side)
  • Second is our own internal message type, which is numeric
    In both cases we want to deliver to client a string. If he didn't defined his own message type, we return "mapped" version of our internal message type.
    Either way we also allow access to "raw" values using PNMessageType.message_type and PNMessageType.pn_message_type properties. And i guess enum doesn't have such possibilities

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time we need to explain this I'm wondering if that's actually a good decision. Especially when you put it like that:

wrapper around two independent message types.

@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from 52774ec to da677baCompareMarch 1, 2023 09:23
@@ -0,0 +1,27 @@
classPNMessageType:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time we need to explain this I'm wondering if that's actually a good decision. Especially when you put it like that:

wrapper around two independent message types.

@seba-alnseba-alnforce-pushed the feature/additional-space-and-message-type-fields branch from d443f63 to c35ba87CompareMarch 29, 2023 09:00
Copy link

@kleewhokleewho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good

Sign up for freeto 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

@seba-aln@kleewho@marek-lewandowski