Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Should BeforeSendCallback have non-nullable hint? #1478

Closed
fzyzcjy opened this issue May 25, 2023 · 6 comments · Fixed by #1574
Closed

Should BeforeSendCallback have non-nullable hint? #1478

fzyzcjy opened this issue May 25, 2023 · 6 comments · Fixed by #1574
Assignees
@denrase
Labels
Breaking-change Platform: Dart repository maintenance
Milestone
8.0.0

Comments

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 25, 2023 •
edited
Loading

When implementing the attachment mechanism suggested by @marandaneto in #683, I realize BeforeSendCallback has a nullable Hint. If the passed-in hint is null, I have no way to attach an attachment. On the other hand, I quickly glanced through the source code and it seems that it guarantees it is not null (by providing a default value). So shall we mark it as non-null?

Platform

Dart

Obfuscation

Enabled

Debug Info

Enabled

Doctor

Version

latest

Steps to Reproduce

Expected Result

Actual Result

Are you willing to submit a PR?

None
@marandaneto marandaneto added this to the 8.0.0 milestone May 25, 2023
@marandaneto
Copy link
Contributor

True, I don't recall why we've not made it non-nullable, @denrase maybe?
We can do it in the next major since it's a breaking change.
Thanks for the feedback @fzyzcjy

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 25, 2023

You are welcome and looking forward to the next release!

@marandaneto
Copy link
Contributor

Hint should not be nullable, but it's an optional parameter anyway (so a user isn't forced to pass anything), the SDK automatically creates a Hint if necessary.
This should base on the v8 branch.

@denrase
Copy link
Collaborator

denrase commented Jul 25, 2023

@marandaneto Don't recall either, maybe took the API design from another SDK and didn't think about this case. 🤔

@denrase denrase mentioned this issue Jul 31, 2023
Merged
6 tasks
@denrase
Copy link
Collaborator

denrase commented Sep 4, 2023

@fzyzcjy Thanks for bringing this up. The new API will be part of the 8.0.0 release.

@denrase denrase closed this as completed Sep 4, 2023
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Sep 4, 2023

🎉

@MrLightful MrLightful mentioned this issue Apr 22, 2024
Merged
3 tasks
buenaflor added a commit to getsentry/sentry-docs that referenced this issue Apr 23, 2024
@MrLightful @buenaflor
Fix beforeSend signature in code snippets for dart platforms (#9770)
903b00b 
* Fix beforeSend signature in code snippets for dart platforms

A breaking change introduced in v8.0.0: getsentry/sentry-dart#1478

* Update beforeBreadcrumb signature for dart platforms (pt1)

Co-authored-by: Giancarlo Buenaflor <giancarlobuenaflor97@gmail.com>

* Update beforeBreadcrumb signature for dart platforms (pt2)

Co-authored-by: Giancarlo Buenaflor <giancarlobuenaflor97@gmail.com>

---------

Co-authored-by: Giancarlo Buenaflor <giancarlobuenaflor97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@denrase denrase

Labels
Breaking-change Platform: Dart repository maintenance
Projects
Mobile & Cross Platform SDK
Archived in project
Milestone
8.0.0
4 participants
@kahest @denrase @fzyzcjy @marandaneto