-
-
Notifications
You must be signed in to change notification settings - Fork 168
feat(types): Add custom variant to AttachmentType
#916
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(types): Add custom variant to AttachmentType
#916
Conversation
|
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.
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 theAttachmentType
enum - Updates the
as_str
method 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)] |
Copilot
AI
Oct 7, 2025
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.
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.
Copilot uses AI. Check for mistakes.
filename = self.filename, | ||
length = self.buffer.len(), | ||
at = self.ty.unwrap_or_default().as_str(), | ||
at = self.ty.clone().unwrap_or_default().as_str(), |
Copilot
AI
Oct 7, 2025
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.
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(), |
Copilot uses AI. Check for mistakes.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.