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

Compatibility mode #68

Merged
merged 4 commits into from
Jul 1, 2015
Merged

Compatibility mode #68

merged 4 commits into from
Jul 1, 2015

Conversation

iconara
Copy link
Member

@iconara iconara commented Jun 30, 2015

This adds an option to MessagePack.pack and MessagePack::Packer.new that makes the packer work in what the MessagePack specification calls "compatibility mode" – it disables the bin and str8 types.

This should solve #67.

Please note that I know next to nothing about writing Ruby C extensions. Someone, perhaps @tagomoris, should look at that and give me hints to how to do it properly.

I'm not super happy about calling the option :compatibility_mode, so if there are any other suggestions I would be happy to change it.

iconara added 4 commits June 30, 2015 15:49
When passing `compatibility_mode: true` to MessagePack.pack or `MessagePack::Packer.new` the encoder uses the old MessagePack spec and avoids the bin and str8 types.
When passing `compatibility_mode: true` to MessagePack.pack or `MessagePack::Packer.new` the encoder uses the old MessagePack spec and avoids the bin and str8 types.
These tests should probably be used for the MRI implementation too.
@tagomoris
Copy link
Member

How about compat for option name? The word compat is used for such cases historically.
And now I'm checking code.

@iconara
Copy link
Member Author

iconara commented Jul 1, 2015

I must say I don't see any benefit in using the shorter form "compat" over "compatibility mode". I was thinking of something that describes it better, that explains what is it compatible with. I want to express "backwards compatible" and "don't use new features" and "raw not bin or str8".

@tagomoris
Copy link
Member

We don't have appropriate name for what it is compatible with...
IMO just saying compat means what is compatible with behavior of older versions. But I agree with your comment: explaining longer name is better than shorter one.

@tagomoris
Copy link
Member

@iconara I added complete cruby implementation. please merge #68.

@iconara iconara merged commit de44c22 into master Jul 1, 2015
@iconara
Copy link
Member Author

iconara commented Jul 1, 2015

Merged to master.

@frsyuki
Copy link
Member

frsyuki commented Jul 1, 2015

👍

@tagomoris
Copy link
Member

#70 (instead of #69) also merged. I'll release v0.6.1.

@iconara
Copy link
Member Author

iconara commented Jul 1, 2015

That's great, thanks for the review and the cruby code.

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.

3 participants