Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cluster array bug #6

Closed

Conversation

IlyaVino
Copy link

This is an incomplete fix for #5 so that you can have access to what I've done so far.

This is my first time contributing, so please let me know if this is not the best way to share work/if there are any best practices I've missed.

-Cluster array with array in it (fail)
-Cluster with array in it (pass)
-Cluster array with simple types (pass)
-Cluster with cluster array (fail)
Initial fail appears to be in Serializer.PlainText\PlainTextToVariant.vi
…mment inside cluster and array case.

-New bug where numeric values affected by above bug lose their values
-Added tests to check that numerics preserve their values
Tests that failed:
-Cluster with array - numeric array
-Cluster with cluster array - nested numeric
-Array of Clusters - numeric array

Likely source of bug:
When unmarshaling, Serializer.PlainText.lvclass:PlainTextToVairant.vi cannot preserve numeric type.
Conversion of kvp variants to strict variants in OpenVariant.lvlib:Variant_KVP to Strict Variant.vi requires matching numeric types under the true -> default -> array (x40) case.
@francois-normandin
Copy link
Member

Thanks for the submission @IlyaVino
This is the right way to do it, although I'd like to make a Unit Test before and after the fix, so that I can evaluate whether or not I can apply this PR to the main branch.
Can you provide some more context in the comments of issue #5 ? If you have a unit test readily available, that would be even better.

@francois-normandin
Copy link
Member

@IlyaVino
I`ve just pushed updates to DataManipulation (1.5.0) and OpenSerializer (1.2.0) to VIPM.io
It should make it public soon.

I will close your pull request as the complete fix required intervention in the DataManipulation library, so your fix would have stayed incomplete. Thanks for going through the process, I really appreciate.

In the meantime, if you'd like to give it a spin before it is available on vipm.io, you can go to the release sections of both repos and download the VIP files from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants