-
-
Notifications
You must be signed in to change notification settings - Fork 277
add a Message type to add a logEntry to Event #107
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
Conversation
dart/lib/src/protocol/message.dart
Outdated
| } | ||
|
|
||
| @override | ||
| bool operator ==(Object other) => |
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.
We haven't been implementing these jn thr protocol. Is it really worth 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.
Not really, I thought maybe it could in some tests, but the existing tests test the serialization equality, so I guess it's not necessary.
dart/lib/src/protocol/message.dart
Outdated
| @override | ||
| bool operator ==(Object other) => | ||
| identical(this, other) || | ||
| other is Message && | ||
| runtimeType == other.runtimeType && | ||
| formatted == other.formatted && | ||
| message == other.message && | ||
| params == other.params; | ||
|
|
||
| @override | ||
| int get hashCode => formatted.hashCode ^ message.hashCode ^ params.hashCode; |
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.
why do we need that if all the other protocols don't have it? unless we want to unit test references etc
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 thought about it, but as it's not done for now in the protocol, I removed them for now.
marandaneto
left a comment
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
📢 Type of change
📜 Description
Add a new Message class
💡 Motivation and Context
The Message Interface carries a log message that describes an event or error. Optionally, it can carry a format string and structured parameters. This can help to group similar messages into the same issue. cf. https://develop.sentry.dev/sdk/event-payloads/message/
fix #100
💚 How did you test it?
I updated the existing tests to verify all the message properties are correctly serialized
📝 Checklist
🔮 Next steps