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

Command set versionning (fix #34) #40

Merged
merged 6 commits into from
Jan 4, 2015
Merged

Command set versionning (fix #34) #40

merged 6 commits into from
Jan 4, 2015

Conversation

antoinealb
Copy link
Member

first pull request of the year \o/

This implements command set versionning. I am not entierly sure of my implementation details, especially for the tests, so peer review welcomed.

The removed test checked for absence of behavior, which is not
a good test since it will not invalidate if the behaviour is not present
due to something else.
@antoinealb antoinealb changed the title Command set versionning (fix #30) Command set versionning (fix #34) Jan 1, 2015
@msplr
Copy link
Contributor

msplr commented Jan 1, 2015

Why don't you use the COMMAND_SET_VERSION macro/variable for all tests? Isn't it cumbersome to change all tests for every version change?
The rest looks good to me.

@antoinealb
Copy link
Member Author

Well it would change the CRC, so you would have to repair the tests either way. Although the best is probably to compute the CRC automatically.

@msplr
Copy link
Contributor

msplr commented Jan 1, 2015

We should probably add compatibility tests between python and C implementations.
Now we only test each part with itself, which isn't always correct for the actual application.

@antoinealb
Copy link
Member Author

I was thinking about it too, but I don't know if it is worth it. I would do it by making a C to Python binding to the target code and then write all compatibility tests in python.

@msplr
Copy link
Contributor

msplr commented Jan 1, 2015

opened an issue #41 to discuss this topic.

@antoinealb
Copy link
Member Author

It should not be a problem as cmp_read_int handles both signed and unsigned integers (see https://github.com/camgunz/cmp/blob/master/cmp.c#L1202-L1235). I can change it though.

@msplr
Copy link
Contributor

msplr commented Jan 4, 2015

I think this can merge.

msplr pushed a commit that referenced this pull request Jan 4, 2015
@msplr msplr merged commit 57e4abc into cvra:master Jan 4, 2015
@antoinealb antoinealb deleted the feature-messagepack-version branch January 4, 2015 22:51
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