Optional Dictionary to add custom tags when creating a SentryEvent

[mr-sige]

This change is

[asbjornu]

The SentryEvent object was created to avoid constructor overload hell, so I don't understand why we need this?

[mr-sige]

Right now you can add tags using sentryEvent.Tags.Add( one by one.
I was thinking it would be nice if I could pass a Dictionary to the constructor with all the tags I'd like to add. :)

[samukce]

When we add a parameter in constructor it is more hard do maintenance and new parameters in sentryEvent easily could maintain the same pattern (constructor hell.)

Maybe is more interesting create a method like SentryEvent.MergeTags(Dictionary<string, string> tags), this way you could build automated tests for ensuring for instance tags already the same key from the dictionary exist in Tags property.

[asbjornu]

I'm not a fan of having several methods pertaining a property on the class itself; I think it's neater to group the methods within the property instead. That would require an extension method or a new interface for the Tags property. Extension methods are more backwards compatible, so I would prefer to see a solution using them rather than an interface or other alternatives. If that means the Tags property needs to have its setter made internal, that's fine.

[samukce]

Good idea. Using Extension is the better solution.

On Mon, Sep 5, 2016 at 9:40 AM, Asbjørn Ulsberg notifications@github.com
wrote:

I'm not a fan of having several methods pertaining a property on the class
itself; I think it's neater to group the methods within the property
instead. That would require an extension method or a new interface for the
Tags property. Extension methods are more backwards compatible, so I
would prefer to see a solution using them rather than an interface or other
alternatives. If that means the Tags property needs to have its setter
made internal, that's fine.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#149 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUhkCUsNbkkTwjL9QGgcEu-WN77WwFRks5qnA3AgaJpZM4JyAVG
.

Regards,

*Samuel Pereira *
Senior Systems Analyst
c. +55 85 9 8883.1368
skype samuel.p.araujo
l. br.linkedin.com/in/samuelpereiraaraujo
g. github.com/samukce

[mr-sige]

Hey Asbjørn!

Can you be a bit more specific, what do you mean by "extension method for
tags property", how would it work, where would it be?

Thanks

-sige

On Mon, Sep 5, 2016 at 4:04 PM Samuel P. notifications@github.com wrote:

Good idea. Using Extension is the better solution.

On Mon, Sep 5, 2016 at 9:40 AM, Asbjørn Ulsberg notifications@github.com
wrote:

I'm not a fan of having several methods pertaining a property on the
class
itself; I think it's neater to group the methods within the property
instead. That would require an extension method or a new interface for
the
Tags property. Extension methods are more backwards compatible, so I
would prefer to see a solution using them rather than an interface or
other
alternatives. If that means the Tags property needs to have its setter
made internal, that's fine.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<
#149 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/ABUhkCUsNbkkTwjL9QGgcEu-WN77WwFRks5qnA3AgaJpZM4JyAVG

.

Regards,

*Samuel Pereira *
Senior Systems Analyst
c. +55 85 9 8883.1368
skype samuel.p.araujo
l. br.linkedin.com/in/samuelpereiraaraujo
g. github.com/samukce

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#149 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFoMbCGP4nZ9iPxl5h5yFm5UnU83C-ugks5qnA7ggaJpZM4JyAVG
.

[asbjornu]

@sigebacsi Since Tags currently is a IDictionary<string, string>, you can create an extension method like so:

public static void Add(this IDictionary<string, string> instance, IDictionary<string, string> dictionary)
{
    foreach (var kvp in dictionary)
    {
        instance.Add(kvp);
    }
}

This method can reside in the SharpRaven library, but seeing how simple it is, I'd say you can just add it to your own code. No need to wait for a new release of SharpRaven to simplify your own code. 😄

[mr-sige]

Okay.
If you don't see it as a useful addition to SharpRaven, the I'll just add it to my code.

[genne]

Any specific reason for the setter of SentryEvents.Tags being internal and not public? It it was public you could write the following code:

new SentryEvent(...)
{
    Tags = myTags
}

[asbjornu]

@genne Good question. It's helpful for SharpRaven itself to know that it can't be null, and I imagined that exposing an IDictionary<string, string> would give everyone what they needed, but it's of course possible to make the property writable. It already has null protection in place, so I wouldn't oppose a PR doing just that.

[asbjornu]

@sigebacsi Now that #157 is merged, does it give you what you need? If so, can this PR be closed?