Skip to content

Fix: Remove is Enum check to support older Dart versions#635

Merged
marandaneto merged 4 commits into
mainfrom
fix/enum-check
Nov 15, 2021
Merged

Fix: Remove is Enum check to support older Dart versions#635
marandaneto merged 4 commits into
mainfrom
fix/enum-check

Conversation

@marandaneto

@marandaneto marandaneto commented Nov 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

📜 Description

Fix: Remove is Enum check to support older Dart versions

💡 Motivation and Context

Closes #634

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

#636

@codecov-commenter

codecov-commenter commented Nov 15, 2021 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.28%. Comparing base (3106a46) to head (8eda12c).
Report is 1563 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
- Coverage   90.29%   90.28%   -0.02%     
==========================================
  Files          93       93              
  Lines        3041     3035       -6     
==========================================
- Hits         2746     2740       -6     
  Misses        295      295

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marandaneto
marandaneto requested a review from a team November 15, 2021 08:48
marandaneto
marandaneto commented Nov 15, 2021
View reviewed changes
Comment thread dart/lib/src/utils.dart
marandaneto
marandaneto commented Nov 15, 2021
View reviewed changes
Comment thread dart/test/utils_test.dart
Comment on lines +26 to +27
expect(jsonSerializationFallback(SentryOrientation.landscape),
'SentryOrientation.landscape');

@marandaneto marandaneto Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for known SDK types this is not a problem
See https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/protocol/sentry_device.dart#L178-L194
this is only for unknown types set dynamically by the user.

@kuhnroyal

kuhnroyal commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

Can we add some way for the user to specify the fallback themselves?

@marandaneto

marandaneto commented Nov 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can we add some way for the user to specify the fallback themselves?

since you are setting types that the SDK don't know during ser/deser, instead of setting the object itself, you could simply set the serialized version already.
eg
event.contexts['myEnumType'] = describeEnum(myEnum);
or mutating the event with your own description of the object type during beforeSend https://docs.sentry.io/platforms/dart/configuration/filtering/

bruno-garcia
bruno-garcia approved these changes Nov 15, 2021
View reviewed changes

@bruno-garcia bruno-garcia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No way to ifdef?

philipphofmann
philipphofmann reviewed Nov 15, 2021
View reviewed changes

@philipphofmann philipphofmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard for me to evaluate if this change is fine as I have no experience with Dart. The only thing I can point out is that we should add tests for this if possible. If there are tests, why didn't we have to change them?

Comment thread dart/lib/src/utils.dart
@marandaneto

marandaneto commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

No way to ifdef?

no afaik dart-lang/sdk#7238

@marandaneto

marandaneto commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

It's hard for me to evaluate if this change is fine as I have no experience with Dart. The only thing I can point out is that we should add tests for this if possible. If there are tests, why didn't we have to change them?

tests were modified, see utils_test.dart

philipphofmann
philipphofmann approved these changes Nov 15, 2021
View reviewed changes

@philipphofmann philipphofmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@marandaneto
marandaneto merged commit fe969f9 into main Nov 15, 2021
@marandaneto
marandaneto deleted the fix/enum-check branch November 15, 2021 09:50
@github-actions

github-actions Bot commented Nov 15, 2021

Copy link
Copy Markdown
Contributor
Fails
🚫 Please consider adding a changelog entry for the next release.
`Instructions and example for changelog`$

Please add an entry to CHANGELOG.md` to the "Unreleased" section under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Remove is Enum check to support older Dart versions(#635)

If none of the above apply, you can opt out by adding _#skip-changelog_ to the PR description.

Generated by 🚫 dangerJS against 8eda12c

@ueman

ueman commented Nov 15, 2021

Copy link
Copy Markdown
Collaborator

Can we add some way for the user to specify the fallback themselves?

That should be pretty straight forward to implement, in the SDK.
I think jsonSerializationFallback is currently a top level function somewhere but it could be changed to a property of SentryOptions and be customizable.

@marandaneto

marandaneto commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

Can we add some way for the user to specify the fallback themselves?

That should be pretty straight forward to implement, in the SDK. I think jsonSerializationFallback is currently a top level function somewhere but it could be changed to a property of SentryOptions and be customizable.

it'd be easy, but not sure if it's worth it, there are many ways of solving it on client-side only, another one would be overwriting toString() plus the ones mentioned above #635 (comment)

@kuhnroyal

kuhnroyal commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

I agree, it's fine to handle manually on my side. Just need to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bruno-garcia bruno-garcia bruno-garcia approved these changes
@philipphofmann philipphofmann philipphofmann approved these changes
@brustolin brustolin Awaiting requested review from brustolin
@denrase denrase Awaiting requested review from denrase denrase is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

version 6.1.1 Error: 'Enum' isn't a type.

6 participants

@marandaneto @codecov-commenter @kuhnroyal @ueman @bruno-garcia @philipphofmann