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

Fix variants, add tests #143

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix variants, add tests #143

wants to merge 4 commits into from

Conversation

mvduin
Copy link
Contributor

@mvduin mvduin commented May 17, 2017

They were apparently quite broken...

before:

> unmarshall(marshall("vv", [ ['i', 42], ['s', 'foo'] ]), "vv")
[ [ [ [Object] ], [ 42 ] ], [ [ [Object] ], [ 'foo' ] ] ]

> unmarshall(marshall("v", [ ['ai', [1, 2, 3]] ]), "v")
Error: Invalid struct data

after:

> unmarshall(marshall("vv", [ ['i', 42], ['s', 'foo'] ]), "vv")
[ [ 'i', 42 ], [ 's', 'foo' ] ]

> unmarshall(marshall("v", [ ['ai', [1, 2, 3]] ]), "v")
[ [ 'ai', [ 1, 2, 3 ] ] ]

@sidorares
Copy link
Owner

Thanks Matthijs , I really hope to find time soon to review all pending PRs on this project, was very very busy lately

mvduin added 4 commits May 28, 2017 11:42
This is e.g. for variants, and includes the small optimization for the
simplest cases that's currently done in marshall.
@mvduin mvduin mentioned this pull request May 29, 2017
@mvduin
Copy link
Contributor Author

mvduin commented Aug 4, 2017

Just letting you know that I no longer care about this PR. I wrote my own dbus module instead.

@sidorares
Copy link
Owner

@mvduin Sorry to hear that. I'm happy to add link to your module from readme here

@ghost
Copy link

ghost commented Oct 7, 2017

@sidorares : can this be merged if the conflict is resolved? This is helpful for eslint, so I'd like to merge it before prettifying.

@ghost
Copy link

ghost commented Oct 17, 2017

So obviously that didn't happen, but I'd still like to see a fix for variants. Can you review it @sidorares

@ghost
Copy link

ghost commented Oct 18, 2017

@sidorares : so how about this one?

@gcampax gcampax mentioned this pull request May 31, 2018
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