Simplified subscription usage and updated the TriBool implementation #27
Conversation
| /** | ||
| * \brief An enum for the different accepted values. | ||
| */ | ||
| enum Values |
There was a problem hiding this comment.
Why the plural? I know there are multiple valid values, but it gets weird when you declare a variable of this type: Values value. It really just holds a single value, does it not?
There was a problem hiding this comment.
That's how I usually do it, and I think it's logical to have the type as plural (since there are several accepted values) and then have singular in the instance. But I guess it's a bit subjective :)
| @@ -333,7 +333,7 @@ POCOClient::POCOResult POCOClient::webSocketConnect(const std::string uri, const | |||
| { | |||
| result.addHTTPRequestInfo(request); | |||
| p_websocket_ = new WebSocket(client_session_, request, response); | |||
| p_websocket_->setReceiveTimeout(Poco::Timespan(60000000)); | |||
| p_websocket_->setReceiveTimeout(Poco::Timespan(LONG_TIMEOUT)); | |||
There was a problem hiding this comment.
LONG_TIMEOUT: name is a bit random ;)
There was a problem hiding this comment.
Well, not too random I think since it's a longer timeout than the default timeout. Maybe the timeout should be made configurable in the future.
There was a problem hiding this comment.
I guess my comment was more about why you chose this name. LONG_TIMEOUT is arbitrary, as the name does not convey anything about how long the timeout is. Reading your code I cannot deduce how long things will wait before timing out unless I go and lookup the declaration of the constant.
There was a problem hiding this comment.
Ok, then I misunderstood what you meant and it's true for the DEFAULT_TIMEOUT as well. Anyway, issue #28 should solve this.
| @@ -178,7 +178,7 @@ bool RWSSimpleStateMachineInterface::toggleIOSignal(const std::string iosignal) | |||
| bool result = false; | |||
| int max_number_of_attempts = 5; | |||
|
|
|||
| if (isAutoMode()) | |||
| if (isAutoMode().isTrue()) | |||
There was a problem hiding this comment.
Should something happen / be printed when it isn't?
There was a problem hiding this comment.
The method will return false if it fails, so I don't think it's needed to print anything here. The log can be inspected, if needed, to see what failed.
Co-Authored-By: jontje <[email protected]>
|
Feel free to merge @jontje. |
|
Great and thanks for the review @gavanderhoorn. |
Simplified the RWS subscription usage for when the event content is inconsequential (part of issue Improve the RWS subscription implementation #24)
Updated the TriBool according to:
Reason: TriBool is useful outside of the RWSInterface class, and doesn't really have anything to do with the class per se.
Reason: The implicit bool conversion resulted in the following unintended behavior:
TriBool tri_bool(TriBool::FALSE_VALUE);tri_bool == TriBool::UNKNOWN_VALUE // Resulted in false. Ok!TriBool::UNKNOWN_VALUE == tri_bool // Resulted in true. Not ok!