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

ws:// processing at Notifier library #1955

Merged

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Mar 28, 2016

Part of #1669

Unfortunatelly, make ft is broken in feature/1181_websocket due to current WS implementation in that branch, I cannot assure this modification doesn't break anything...

@fortizc

@@ -30,6 +30,18 @@
#include "ngsiNotify/senderThread.h"


// FIXME P11 #1669: remove this stub when the actual sendNotifyContextRequestWs() gets developed a the end. @fortizc please take care of this
// in the ws library
int sendNotifyContextRequestWs(const std::string& subId, const std::map<std::string, std::string>& headers, const std::string& data)
Copy link
Member Author

Choose a reason for hiding this comment

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

At the begining I proposed (at #1669) to use bool as return value for this function. However, in order to align with httpRequestSent() signature used at startSenderThread() function, I think it should be better to use int the the usual semantics: 0 -> everythin ok, not 0 -> error.

(I have edited signature at #1669 (comment) has been changed void -> int)

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

@fortizc
Copy link
Contributor

fortizc commented Mar 28, 2016

LGTM !

@frbattid frbattid merged commit 3bbdc55 into feature/1181_websockets Mar 28, 2016
@frbattid frbattid deleted the task/1669_ws_notifications_internal_parts branch March 28, 2016 15:48
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