feat(types): Add custom variant to AttachmentType#916
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for custom attachment types by extending the AttachmentType enum to include arbitrary string values. This allows users to send attachments with custom type identifiers beyond the predefined Sentry attachment types.
- Adds a
Custom(String)variant to theAttachmentTypeenum - Updates the
as_strmethod to work with borrowed values and handle custom types - Includes deserialization tests for the new custom attachment functionality
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| /// The different types an attachment can have. | ||
| #[derive(Debug, Copy, Clone, Eq, PartialEq, Deserialize)] | ||
| #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] |
There was a problem hiding this comment.
Removing Copy trait from the enum will impact performance for small operations. Consider keeping Copy and use Cow<'static, str> for the Custom variant to maintain zero-cost abstractions for predefined types while supporting custom strings.
| filename = self.filename, | ||
| length = self.buffer.len(), | ||
| at = self.ty.unwrap_or_default().as_str(), | ||
| at = self.ty.clone().unwrap_or_default().as_str(), |
There was a problem hiding this comment.
Unnecessary clone operation. Since as_str now takes &self, you can use self.ty.as_ref().unwrap_or(&AttachmentType::default()).as_str() to avoid cloning the attachment type.
| at = self.ty.clone().unwrap_or_default().as_str(), | |
| at = self.ty.as_ref().unwrap_or(&AttachmentType::default()).as_str(), |
There was a problem hiding this comment.
maybe a .as_ref().map(|a| a.as_str()).unwrap_or_default() would be even more concise (and avoid the clone).
There was a problem hiding this comment.
This .as_ref().map(|a| a.as_str()).unwrap_or_default() will yield and empty str rather than event.attachment so not sure we want to change this (at least in this PR).
There was a problem hiding this comment.
I can still do the explicit unwrap_or though that one should work.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #916 +/- ##
==========================================
+ Coverage 73.76% 73.79% +0.02%
==========================================
Files 64 64
Lines 7533 7544 +11
==========================================
+ Hits 5557 5567 +10
- Misses 1976 1977 +1 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AttachmentType
| filename = self.filename, | ||
| length = self.buffer.len(), | ||
| at = self.ty.unwrap_or_default().as_str(), | ||
| at = self.ty.clone().unwrap_or_default().as_str(), |
There was a problem hiding this comment.
maybe a .as_ref().map(|a| a.as_str()).unwrap_or_default() would be even more concise (and avoid the clone).
Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>
We would like to be able to send a custom attachment type, this makes it such that this is possible.