search

mradziwo / MsgPack_LabVIEW

It's like JSON, but fast and small…and LabVIEW! – msgpack.org[LabVIEW]
http://msgpack.org
MIT License
2 stars 4 forks source link

Some bug fixes... #2

Closed zoyo-de closed 6 years ago

zoyo-de commented 6 years ago

Hi, I did some changes on your code. Maybe you want to have a look at it.

I fixed some small bugs:

added a function:

and did some cosmetical changes:

kind regards, Christian

mradziwo commented 6 years ago

Sorry for late reply - I was on holidays during this time.. thank you for the fixes, I will review them during this week and add to repo.

Thanks for contribution!

Best regards, Michał Radziwon

On 12 May 2018 at 23:15, zoyo-de notifications@github.com wrote:

Hi, I did some changes on your code. Maybe you want to have a look at it.

I fixed some small bugs:

  • the reported issue at array encoding
  • some wrong constants for integer encoding

added a function:

  • added a case for void->nil encoding in encodeObject.vi to be able to encode empty variants

and did some cosmetical changes:

  • detached the compiled code from source in project settings for better git compability (no changed VIs without changes in source code by automatic recompilation)
  • added case structures around around every VI-code that could generate own errors. Usually VIs should not mask previous errors.

kind regards, Christian

You can view, comment on, or merge this pull request online at:

https://github.com/mradziwo/MsgPack_LabVIEW/pull/2 Commit Summary

  • signed-int-encoding fixed, array encoding fixed, compiled code detached
  • Nil-Handling added
  • Added case-structure to all VIs to propagate errors unchanged.

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mradziwo/MsgPack_LabVIEW/pull/2, or mute the thread https://github.com/notifications/unsubscribe-auth/AI1rE9sjoCrqEfRA1EZXahjqTrcXZ7DSks5tx1EJgaJpZM4T8kNL .

mradziwo commented 6 years ago

Hi Zoyo,

You have put substantial piece of good work to the project! Great thenak for contributing.

I think you have missed to stage and commit SubVis\mapheader.vi - could you add it and then all pack will get merged

zoyo-de commented 6 years ago

Oh sorry! The VI was added.

But this last commit was actually not part of the pull request because it's to new and I'm not shure about the API. Some thoughts on this conversion:

The cluster->map conversion makes it easy to access the fields on the other side when converted to a dict/hash (I use this with Python on the other end). But when you have to transfer an array of clusters of same type it is an unnecessary overhead. For this it might be better to transfer only the values like before and an additional array with the cluster names. For this a switch is needed for encodeCluster.vi to choose between cluster elements and names when Cluster-To-Map is False. Or better use an enum for the three conversion modes "cluster -> name array", "cluster -> value array" and "cluster -> map". I have to think a little about it. Esp. how to handle this from the toplevel encodeObject.vi and what happens when you have a cluster inside of a cluster...

kind regards, Christian

mradziwo commented 6 years ago

That is why I kind of skipped MAP type - as it is not trivial to throw it to LabVIEW - which is strictly typed language. The option I was considering was to convert map to variant with attributes. Though to make it nestable - it will require to have attributes of variant type - which in turn can take such attributes - that's not an elegant solution in LabVIEW.

Alternatively one can attempt to dynamically create clusters that will be casted to variant at output. This approach though is hell to maintain in labview, when you will want to add/change map message (been there, done that - it sucks).