Conversation
| int on_subscribe; | ||
| int on_subscribe_v5; | ||
| int on_unsubscribe; | ||
| int on_unsubscribe_v5; |
There was a problem hiding this comment.
All the callbacks can stay the same right? you can't have v3 and v5 in the same client object, so there's no confusion there.
There was a problem hiding this comment.
libmosquitto will call both callbacks when i.e. mosquitto_connect_callback_set and mosquitto_connect_v5_callback_set have been called before.
There was a problem hiding this comment.
is that actually meaningful? a client can only be one protocol at a time, so being able to register two, and getting two callbacks just seems like complexity for nothing
There was a problem hiding this comment.
I think the idea for the v5 api design of libmosquitto is, that old code can stay unmodified and meanwhile can be extended with v5 functionality by just adding postfixed _v5 functions to your codebase.
Now since Lua is much more flexible than C, because of optional arguments and so on, there could probably also be a method to reach the same goal with just extending the existing API. But this would mean, that from there on this lib is more than just a transparent wrapper for libmosquitto and would probably involve some additional documentation work since you can not simply refer to the mosquitto.h and say this is our API.
I would say the biggest work in this PR was the properties parsing back and forth so changing the design decision on this point might not be catastrophic.
I like the current approach more because it is way more explicit but I am open to your final decision :)
lua-mosquitto.c
Outdated
| { | ||
| ctx_t *ctx = ctx_check(L, 1); | ||
| enum mosq_opt_t option = luaL_checkint(L, 2); | ||
| enum mosq_opt_t option = luaL_checkinteger(L, 2); |
There was a problem hiding this comment.
this looks like an unrelated change? why do you need this?
There was a problem hiding this comment.
I run into an compile error, because I forgot to set LUA_COMPAT_APIINTCASTS.
Change removed
lua-mosquitto.c
Outdated
| {"loop_forever", ctx_loop_forever}, | ||
| {"loop_start", ctx_loop_start}, | ||
| {"loop_stop", ctx_loop_stop}, | ||
| {"socket", ctx_socket}, |
There was a problem hiding this comment.
all the whitespace changes here make it harder to follow :|
There was a problem hiding this comment.
I tried to revert the whitespace.
|
It looks like you've got v5 support in the general functions, and also _v5 suffixed functions? Does that really make it easier to use? In general this looks ok though, I've not tried any of it out though. |
Yes you are right, I wanted to share some code but since this already lead to confusions, I changed the functionality now, so that the old functions are not touched. |
Oh, I'm not against the combined :) I actually suggested it in my issue discussing how this should be done, it just seemed to be not what you had said you had done :) |
|
Hello @karlp , I am wondering If I could provide you anything more in order to help you with the review and merge. |
2 participants
Fixes #26
I decided to to create separate functions for all v5 functionality because of the following reason:
While this PR is open I will check further how testing could be improved and see if the documentation needs additional updates.