-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat: Support modifying attachments after init #433
base: master
Are you sure you want to change the base?
Conversation
Moves the attachments to the scope, and adds `sentry_add_attachment` and `sentry_remove_attachment` and wstr variants that modify this attachment list after calling init. Attachments are identified by their path.
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.
Thanks @Swatinem 💯. The API seems sufficient for synching attachments from Java to native.
I'm sorry I was wrong. I think we need some more changes to this PR.
In the SDK API Evolution meeting, we decided it is fine to just copy the byte array to native with the tradeoff of increasing the memory footprint. |
* | ||
* See the NOTE on attachments above for restrictions of this API. | ||
*/ | ||
SENTRY_API void sentry_add_attachment(const char *path); |
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.
Will we always want this to be void? Does a status return make any sense now or in the future?
* | ||
* See the NOTE on attachments above for restrictions of this API. | ||
*/ | ||
SENTRY_API void sentry_remove_attachment(const char *path); |
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.
While an idempotent API is good, would a return from which you can tell whether anything happened or not be useful?
* API Users on windows are encouraged to use `sentry_add_attachmentw` instead. | ||
* | ||
* See the NOTE on attachments above for restrictions of this API. | ||
*/ |
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.
Does it make sense to explicitly say something about the ownership of *path
? (same for remove attachment)
@@ -364,7 +364,9 @@ sentry_envelope_serialize(const sentry_envelope_t *envelope, size_t *size_out) | |||
|
|||
sentry__envelope_serialize_into_stringbuilder(envelope, &sb); | |||
|
|||
*size_out = sentry__stringbuilder_len(&sb); | |||
if (size_out) { |
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.
nice 😄
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Moves the attachments to the scope, and adds
sentry_add_attachment
andsentry_remove_attachment
and wstr variants that modify this attachmentlist after calling init. Attachments are identified by their path.
Attachments are still based on file paths and loaded lazily, this does not add support for attachments based on in-memory buffers.