Closed maxired closed 12 years ago
baalexander
commented
12 years ago Thanks. This looks good.
In regards to the message.fields and message.constants, is that like what we have now, but moving constant fields to message.constants instead of being in message.fields?
And, can you explain the helper functions part more? Which functions are you referring to? Are you saying these functions should belong to Message prototype?
maxired
commented
12 years ago "is that like what we have now, but moving constant fields to message.constants instead of being in message.fields?" Yes, something like that ;)
About the function like getMessageType, extractFields or even prepareMD5, the problem is that they behave like static functions. To be able to compute the MD5, I needed informations about the object for which we compute (not only the content of the file, but also the package of the object that we are building). That's why I think it should be more object related.
It is your choice to choose which function has to be public (in the prototype) or not. You can also put everything in the prototype and use the "" conventions (method whose name is prefix by means private)
Otherwise, another solution is to have a bigger use of closure or use the module pattern. I'm not yet confident enough in JS to give you pro and cons for thoses solutions.
baalexander
commented
12 years ago @maxired - I just committed a change so that message fields will have the message class as a field property.
For example, geometry_msgs/Twist's field[0].messageType will be the geometry_msgs/Vector3 class. This cleaned up the prepareMD5 function since it can just do field.messageaType.md5 + ' ' + field.name without any extra message fetching.
I also needed the message classes fetched ahead of time to make subscribing to messages performant.
Hi @baalexander, here a pull request for https://github.com/baalexander/rosnodejs/issues/24
It fix the MD5 computation of files and added some test. It seems to be working well. Maybe i will imporve it, but I wanted to provide you very quickly a first version.
One things that I noticed during the implentation, is that others implementations tends to have a message.field for every dynamic field and message.contants for constancts. We probably also should do it this way.
Moreover, I found it annoying that the helper functions in libMessages.js are not linked to the object that we are building. So it is difficult to share a context and lots of things has to be shared within the paramters. I don't think this is a good things.