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

mmtf::encodeToMap #12

Merged
merged 4 commits into from
Jul 12, 2018
Merged

mmtf::encodeToMap #12

merged 4 commits into from
Jul 12, 2018

Conversation

speleo3
Copy link
Collaborator

@speleo3 speleo3 commented Jul 10, 2018

mmtf::encodeToMap is exactly like mmtf::encodeToStream, but without the final call to msgpack::pack. This allows an application to add custom fields to the map before it's packed.

Example which adds PyMOL's color and representation as custom fields:

    msgpack::zone _zone;

    auto data_map = mmtf::encodeToMap(data, _zone);
    data_map["pymolRepsList"] = msgpack::object(repsList, _zone);
    data_map["pymolColorList"] = msgpack::object(colorList, _zone);

    std::stringstream stream;
    msgpack::pack(stream, data_map);

@gtauriello
Copy link
Collaborator

Thanks for the extra convenience function. Looks very useful and I just changed some style issues (and fixed an old bug).

It triggers a semi-philosophical question though: Shall we drop C++-03 support? This would enable the use of "auto" everywhere but we would have to adapt the README.

So far everything apart from unit tests could compile and run without a C++-11 compatible compiler. Now with this change, we would require at least partial C++-11 support (namely the auto type). The cmake files were actually already enforcing to enable the auto-type everywhere. On our machines, we use gcc 4.8.4 or newer so we are good. The oldest compiler I could find on our systems was gcc 4.6.4 and even that one works with the given cmake setup (apart from unit tests). The only thing that fails there is the compile_target script in examples.

@speleo3
Copy link
Collaborator Author

speleo3 commented Jul 10, 2018

Sorry for not paying attention that C++03 is the current supported standard. Removing the auto keyword and making this C++03 compatible is trivial, I can certainly do that if it's a concern.

I'd also be fine with dropping C++-03 support.

Regarding style issues: Do you use clang-format? If yes, can you add your .clang-format to the repo?

@gtauriello
Copy link
Collaborator

I never used clang-format so I don't know much about it. Generally, I never looked into strictly defining a coding style. I prefer to just keep files consistent, but I never liked to be super strict there. Not sure how well high-level concepts like separating signatures from implementations and documenting public functions can be captured by a coding style. These are the things I care more about and I don't want to get lost into discussions on where to put brackets and line continuations.

@speleo3
Copy link
Collaborator Author

speleo3 commented Jul 10, 2018

thanks, sounds good. I wasn't suggesting to enforce a style, I was just wondering if there is something that could help streamline pull requests. And yes, clang-format is only about whitespace, it won't reorganize your signatures.

@speleo3 speleo3 mentioned this pull request Jul 12, 2018
Copy link
Collaborator

@gtauriello gtauriello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me and even work with C++-03 now, so we can keep the README as is.

@gtauriello gtauriello merged commit e77033e into master Jul 12, 2018
@gtauriello gtauriello deleted the encodeToMap branch July 12, 2018 13:53
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