Skip to content

Conversation

vveljko
Copy link
Contributor

@vveljko vveljko commented Aug 12, 2019

Support for the Signals is added. Signals are like messages except
that new signal on the channel pushes out (remvoes) the previous
signal (there is no history). Signals also can't have metadata and
compressing them is not supported.

On the receiving side, if using subscribe V2, user can see if the
received message is a signal or a (regular) message. This prompted
completing the support for subscribe V2 in C++ & Qt.

Other than that, some smaller code cleanups (removing warnings
from various code checkers/analyzers).

Support for the Signals is added. Signals are like messages except
that new signal on the channel pushes out (remvoes) the previous
signal (there is no history). Signals also can't have metadata and
compressing them is not supported.

On the receiving side, if using subscribe V2, user can see if the
received message is a signal or a (regular) message. This prompted
completing the support for subscribe V2 in C++ & Qt.

Other than that, some smaller code cleanups (removing warnings
from various code checkers/analyzers).
@pubnub pubnub deleted a comment Aug 12, 2019
@vveljko
Copy link
Contributor Author

vveljko commented Aug 12, 2019

Codacy complains about constructors with one argument that are not "explicit", but, we actually want them to be implicit. Unfortunately, there is no way to silence Codacy on this...

@vveljko vveljko requested a review from davidnub August 12, 2019 08:22
@@ -0,0 +1,160 @@
/* -*- c-file-style:"stroustrup"; indent-tabs-mode: nil -*- */

Choose a reason for hiding this comment

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

Same comments as pbcc_entity_api.c

jpresult = pbjson_get_object_value(&el, "e", &found);
if (jonmpOK == jpresult) {
if (pbjson_elem_equals_string(&found, "1")) {
rslt.is_signal = true;

Choose a reason for hiding this comment

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

We will have more message types going forward. For example e=2 => ObjectsType and e=3 => MessageActionsType

Not sure if we should have a boolean for each type or just an enum of TYPES.

#define PUBNUB_USE_ADVANCED_HISTORY 0
#endif

#if !defined(PUBNUB_USE_ENTITY_API)

Choose a reason for hiding this comment

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

Objects API

#if PUBNUB_USE_ADVANCED_HISTORY
#include "core/pbcc_advanced_history.h"
#endif
#if PUBNUB_USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

#else
, dont_parse /* PBTT_MESSAGE_COUNTS */
#endif
#if PUBNUB_USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

/** The message metadata, as published */
struct pubnub_char_mem_block metadata;
/** is the message a signal(, or published) */
bool is_signal;

Choose a reason for hiding this comment

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

This might be better as an enum of types, we will have the following types: publish, signal, objects, message_actions and probably more in the future

cpp/posix.mk Outdated
USE_ADVANCED_HISTORY = 1
endif

ifndef USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

USE_ADVANCED_HISTORY = 1
endif

ifndef USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

openssl/posix.mk Outdated
USE_ADVANCED_HISTORY = 1
endif

ifndef USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

#define PUBNUB_USE_ADVANCED_HISTORY 1
#endif

#if !defined(PUBNUB_USE_ENTITY_API)

Choose a reason for hiding this comment

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

Objects API

posix/posix.mk Outdated
USE_ADVANCED_HISTORY = 1
endif

ifndef USE_ENTITY_API

Choose a reason for hiding this comment

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

Objects API

#define PUBNUB_USE_ADVANCED_HISTORY 1
#endif

#if !defined(PUBNUB_USE_ENTITY_API)

Choose a reason for hiding this comment

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

Objects API

#define PUBNUB_USE_ADVANCED_HISTORY 1
#endif

#if !defined(PUBNUB_USE_ENTITY_API)

Choose a reason for hiding this comment

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

Objects API

collection of rest API features that enables "CRUD"(Create, Read, Update and Delete)
on two new pubnub objects: User and Space, as well as manipulating connections
between them. */
#define PUBNUB_USE_ENTITY_API 1

Choose a reason for hiding this comment

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

Objects API

Copy link

@davidnub davidnub left a comment

Choose a reason for hiding this comment

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

There are some comments in the Review:

  • Typos
  • Entity -> Objects
  • Probably remove Objects until we release that

@davidnub
Copy link

Also Codacy requires fixes

@vveljko
Copy link
Contributor Author

vveljko commented Aug 12, 2019

Also Codacy requires fixes

As explained in a comment, Codacy is simply wrong here, it complains about some constructors not being explicit, but, we actually want these to be implicit.

@davidnub
Copy link

Also Codacy requires fixes

As explained in a comment, Codacy is simply wrong here, it complains about some constructors not being explicit, but, we actually want these to be implicit.

Can we just modify the rule that Codacy is using or add a "disable" comment to that section for cppcheck to ignore?

Additionally what are your thoughts about message type as an enum vs a bunch of flags per type?

* Entity API -> Objects API
* pubnub_signal() does not a HTTP verb/method support. It just
  GETs it.
* Prepare to support other message types (than publish and signal)
* Add pubnub_stop() for graceful shutdown when using the callback
  interface
@pubnub pubnub deleted a comment Aug 17, 2019
@pubnub pubnub deleted a comment Aug 17, 2019
@pubnub pubnub deleted a comment Aug 17, 2019
Copy link

@davidnub davidnub left a comment

Choose a reason for hiding this comment

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

I wish we could fix the rules in Codacy, but otherwise LGTM!

@vveljko
Copy link
Contributor Author

vveljko commented Aug 21, 2019

I wish we could fix the rules in Codacy, but otherwise LGTM!

Working on making a Codacy configuration, but it's poorly documented, so it will take some time to figure out.

@vveljko vveljko merged commit c3b3674 into master Aug 21, 2019
@vveljko vveljko deleted the signals branch August 21, 2019 18:00
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