Skip to content
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

Feature/additional space and message type fields #153

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

seba-aln
Copy link
Contributor

No description provided.

@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from ce4d004 to 1109fad Compare February 9, 2023 10:31
@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from 64eb34a to 6c2e89a Compare February 27, 2023 08:44
@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from 6c2e89a to 00e1b16 Compare February 27, 2023 09:05
cleanup + add acceptance test action
@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from 00e1b16 to 4b4776e Compare February 27, 2023 16:03
pubnub/endpoints/fetch_messages.py Show resolved Hide resolved
params = {'max': int(self._count)}
params = {
'max': int(self._count),
'include_type': 'true' if self._include_message_type else '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' if self._include_message_type else 'false',
'include_message_type': 'true' if self._include_message_type else '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()

if self._include_message_type is not None:
params['include_message_type'] = "true" if self._include_message_type else "false"
if self._include_space_id is not None:
params['include_space_id'] = "true" if self._include_space_id else "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
Contributor 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

pubnub/endpoints/file_operations/publish_file_message.py Outdated Show resolved Hide resolved
@@ -0,0 +1,27 @@
class PNMessageType:
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
Contributor 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

Copy link

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.

pubnub/models/consumer/history.py Outdated Show resolved Hide resolved
@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from 52774ec to da677ba Compare March 1, 2023 09:23
@@ -0,0 +1,27 @@
class PNMessageType:
Copy link

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.

pubnub/models/consumer/message_type.py Outdated Show resolved Hide resolved
tests/acceptance/history/environment.py Show resolved Hide resolved
.github/workflows/run-tests.yml Show resolved Hide resolved
@seba-aln seba-aln force-pushed the feature/additional-space-and-message-type-fields branch from d443f63 to c35ba87 Compare March 29, 2023 09:00
Copy link

@kleewho kleewho 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 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