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

Change message type to type. Type do not have fallback #152

Merged
merged 2 commits into from
Mar 27, 2023

Conversation

kleewho
Copy link
Contributor

@kleewho kleewho commented Mar 24, 2023

No description provided.

@kleewho kleewho requested review from parfeon and Xavrax as code owners March 24, 2023 09:29
@kleewho kleewho force-pushed the CLEN-1159_finalize_message_type branch from e27842d to a25f2ba Compare March 24, 2023 10:17
Copy link
Contributor

@Xavrax Xavrax left a comment

Choose a reason for hiding this comment

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

LGTM - few questions

} else {
histItem.MessageType = ""
o.pubnub.Config.Log.Printf("histResponse message_type nil")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that println expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently this is how this code worked before. I'm not actually changing that. This is how it looks like straight on master

@@ -725,7 +719,7 @@ func processSubscribePayload(m *SubscriptionManager, payload subscribeMessage) {
}
}

func createPNFilesEvent(filePayload interface{}, m *SubscriptionManager, actualCh, subscribedCh, channel, subscriptionMatch, issuingClientID string, userMetadata interface{}, timetoken int64, messageType MessageType, spaceId SpaceId) *PNFilesEvent {
func createPNFilesEvent(filePayload interface{}, m *SubscriptionManager, actualCh, subscribedCh, channel, subscriptionMatch, issuingClientID string, userMetadata interface{}, timetoken int64, typ string, spaceId SpaceId) *PNFilesEvent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a typo?

Suggested change
func createPNFilesEvent(filePayload interface{}, m *SubscriptionManager, actualCh, subscribedCh, channel, subscriptionMatch, issuingClientID string, userMetadata interface{}, timetoken int64, typ string, spaceId SpaceId) *PNFilesEvent {
func createPNFilesEvent(filePayload interface{}, m *SubscriptionManager, actualCh, subscribedCh, channel, subscriptionMatch, issuingClientID string, userMetadata interface{}, timetoken int64, type string, spaceId SpaceId) *PNFilesEvent {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I had plenty of options to choose from, but type is not one of them. It's a reserved keyword so we need to be creative with words :)

@@ -757,7 +751,7 @@ func createPNFilesEvent(filePayload interface{}, m *SubscriptionManager, actualC
Publisher: issuingClientID,
UserMetadata: userMetadata,
SpaceId: spaceId,
MessageType: messageType,
Type: typ,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here?

Suggested change
Type: typ,
Type: type,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As stated in previous comment. Not typo. Keyword. That's just life :D

@kleewho kleewho merged commit 97269a7 into spaceid_messagetype Mar 27, 2023
@kleewho kleewho deleted the CLEN-1159_finalize_message_type branch March 27, 2023 07:28
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.

2 participants