Feature public sentryevent.tags #159
Conversation
|
@asbjornu done. o/ |
To make it easier to include tags directly when constructing a new `SentryEvent`. Related to PR #149.
|
@samukce Excellent! But can you perhaps remove the three merge commits from this PR? This is rather easy to do by resetting this branch to the latest git fetch getsentry
git checkout feature/publicTagsSentryEvent
git reset --hard getsentry/develop
git cherry-pick 8d868bf5417ee8f5824faf1102d05306f4708278
git push --force |
…assign from SentryEvent.Tags
samukce
force-pushed
the
feature/publicTagsSentryEvent
branch
from
8d868bf to
06f59a4
Compare
|
@asbjornu thanks for the feedback. Done. |
| var sentryEvent = new SentryEvent(new Exception("foo")); | ||
| var beforeDictionary = sentryEvent.Tags; | ||
|
|
||
| Assert.That(beforeDictionary, Is.SameAs(sentryEvent.Tags)); |
There was a problem hiding this comment.
I don't quite see the value of this assertion.
There was a problem hiding this comment.
The get/set execute some procedures how instance new dictionary. Some refactor could inject a bug, for instance, a new dictionary, what could losing data.
| { | ||
| var sentryEvent = new SentryEvent(new Exception("foo")) { Tags = null}; | ||
|
|
||
| Assert.That(sentryEvent.Tags, Is.Not.Null); |
There was a problem hiding this comment.
Regarding this. The test case is great, but the "feature" feels a bit weird. I would really prefer if the setter of Tags threw an ArgumentNullException or similar when trying to set it to null. Setting something to null and then making sure it wasn't set to null doesn't really make sense?
There was a problem hiding this comment.
@genne: Good point. Let's do ArgumentNullException, then. PR incoming? 😄
There was a problem hiding this comment.
I think that this api should work similar to the log4net, i mean when any is wrong with the API, this not interfere of the function of the software.
There was a problem hiding this comment.
@samukce While I understand that design principle from a logging framework, there's already quite a few throw statements in there.
There was a problem hiding this comment.
@asbjornu but do you agree that some simple exceptions could be avoided?
There was a problem hiding this comment.
Yes, but in this case, I'm basically ¯\_(ツ)_/¯.
Complementing the PR #149. Some tests case to ensure public assign from SentryEvent.Tags
This change is