-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add missing measurements units #1033
Conversation
/// This is a singe enum because Enhanced Enums in Dart is only available | ||
/// in newer versions. |
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.
Limitation before Dart 2.17 and we cannot do breaking changes just yet.
It's either this or an open String, but this has better UX, so users don't need to find out the units by themselves via docs.
Codecov ReportBase: 90.26% // Head: 90.48% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
+ Coverage 90.26% 90.48% +0.21%
==========================================
Files 114 114
Lines 3556 3572 +16
==========================================
+ Hits 3210 3232 +22
+ Misses 346 340 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 there is a different way of fixing it.
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.
LGTM, as discussed on a call with @marandaneto, that Dart doesn't offer inheritance for enums or method overloads, and that's why we put all types into one enum.
📜 Description
Not as nice as
SentryMeasurementUnit.duration.milliSecond
as some other languages, but it's a limitation of theenum
type in Dart in the current min. supported version, we can refactor that in the future.Changing it to a plain
String
would be a breaking change and would have a poor UX because we'd need to document all the possible values.The current limitation is that you can only set the unit with one of the enum values, which is well covered for most of the cases.
In case somebody wants to use their own metric unit, it's currently not supported, we can refactor this and bump the Dart version if needed.
💡 Motivation and Context
Reason: getsentry/sentry-java#2260 (comment)
💚 How did you test it?
📝 Checklist
🔮 Next steps