fix(binding-mqtt): fix incorrect segment indices and change index and… #976
Conversation
… length values to enums Closes eclipse-thingweb#874 Signed-off-by: Hasan Eroglu <[email protected]>
|
@egekorkan can you take a look |
There was a problem hiding this comment.
Question: Can we come up with a test to avoid regressions in the future?
I am not sure, but maybe some very basic interactions with https://test.mosquitto.org/ ?
There was a problem hiding this comment.
Changes look good, but before approving I agree with Daniel that it is better to have test. You can add one at the https://github.com/eclipse/thingweb.node-wot/blob/master/packages/binding-mqtt/test/mqtt-client-subscribe-test.integration.ts . There is no need to depend on https://test.mosquitto.org/. Finally, I added a question below.
| enum SegmentLength { | ||
| "action" = 3, | ||
| "property" = 4, | ||
| } | ||
|
|
||
| enum SegmentIndex { | ||
| "thingName" = 0, | ||
| "interactionType" = 1, | ||
| "interactionName" = 2, | ||
| "interactionExtension" = 3, | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you explain your trail of thoughts to use an enum to model topic parsing?
There was a problem hiding this comment.
For me it was really easy to mix up numbers while coding so I thought it would be easier to use something self-explanatory. Since topic segments always have the similar order, I think with enums the code becomes more readable.
There was a problem hiding this comment.
Personally I would not have used enums but I will definitely not object if you prefer keeping it.
Note: In other bindings we don't use enums for the same type of hierarchy.
There was a problem hiding this comment.
If it is not a problem I would follow the principles of "less surprise" and "consistency". You can use a prefix or suffix to group semantically const. Sorry, for being picky you did a great job already but I think it is worth doing this small refactor.
There was a problem hiding this comment.
I did the change here: 3952483.
|
I like the changes, thanks @hasanheroglu ! Tests would be indeed very beneficial |
|
To be able to add meaningful tests, #979 needs to be solved first. |
Signed-off-by: Hasan Eroglu <[email protected]>
Signed-off-by: Hasan eroglu <[email protected]>
… length values to enums
Closes #874