-
Notifications
You must be signed in to change notification settings - Fork 85
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
Rework attachments #423
Rework attachments #423
Conversation
Could you review this one @sashacmc? |
75afe45
to
05a53e9
Compare
05a53e9
to
4f4b952
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small notes, others LGTM!
/** | ||
* Enable attachments. | ||
*/ | ||
#ifndef Z_FEATURE_ATTACHMENT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed something, but why the attachment now not a feature and can’t be disabled to reduce code size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the change, the impact on code size should be minimal since all the code is now on the API, and it shouldn't be linked if not used by the application.
So I believe we should remove this token so users have one less config option to worry about
For 1.0, attachments are a simple payload internally and the key, value pair serialization must be done at the app level through the zenoh API. Task list: