weixin_39575054
weixin_39575054
2020-12-31 18:46

Another way to try to be on spec!

Remember cupcake/raven-go#14? It was all about trying to pass additional info into the Capture* methods. Unfortunately, the way I chose to do it (variadics pulled apart by type switches) wasn't type safe, which isn't great.

However, there is an obvious way to make type safety work: instead of variadics, just introduce the ability to merge packets.

Current master
 go
func (client *Client) CaptureMessage(message string, tags map[string]string, interfaces ...Interface) string

Only allows tags and interfaces :frowning:

Bad Variadic Solution
 go
func (client *Client) CaptureMessage(message string, attr ...interface{}) string

Not type safe like at all :weary:

This Pull Request
 go
func (client *Client) CaptureMessage(message string, packetToMerge *Packet) string

The actual signature is a little more descriptive, but this does everything both of us want (I think)! :relaxed:

What's Next

Before I get going on: - Fixing the tests - Writing new tests - Adding the in-code documentation

I want to know what you think of this direction! Like last time, the change isn't as big as it looks. Mainly, Capture* methods now take what used to be a Packet in and merging happens now. Client also dispenses the packets like last time. writer.go nicely demos how this looks:

 go
w.Client.CaptureMessage(string(p), &EventInfo{Level: w.Level, Logger: w.Logger})

I ended up renaming some things to make more sense to me, and factoring things into different files while trying to make sense of the world :wink: but I I did like the changes in the end.

Thanks for looking this over again! Let me know if you have any questions.

Closes #19

该提问来源于开源项目:getsentry/raven-go

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

16条回答

  • weixin_39653733 weixin_39653733 4月前

    +1, I like this approach.

    点赞 评论 复制链接分享
  • weixin_39586353 weixin_39586353 4月前

    Anything I can do to help with this?

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    I'm coming back. I have a clear vision for how I'd like to finish this pull request, so the best thing you all can do to help is be available to give me feedback!

    I am committed to finishing this for the new year. Keep me honest!

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    Made some progress on this today, addressed some feedback. Not ready for a second round of review though; will let you know!

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    Holy passing tests batman! Still not ready for a good set of eyes, should be in a few days. Just wanted to celebrate this finally turning into a check mark. :smile:

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    's awesome API is now implemented! :dancer:

    If you're feeling adventurous, you can dig in and try to look at the code as it is right now. I think I've done what I wanted to do with actual implementation.

    I still need to: - Clean up - Write tests - Improve the README, especially to document the Capture* methods - Improve the examples

    Expect these to come over the next couple days, followed by a request for a thorough review. Since this touches most of the existing code, I'm expecting to have to make changes!

    点赞 评论 复制链接分享
  • weixin_39531604 wdk199512 4月前

    What's the ETA on this PR? Is it going to be merged / continued any time soon? Anything we can do to help?

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    Oops, fixed a big bug. Wasn't merging interfaces. Okay now.

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    Hey , let me know what you think of all this! Thanks!

    点赞 评论 复制链接分享
  • weixin_39653733 weixin_39653733 4月前

    Sorry for not getting to this quickly, it might be a couple more days. My week is really busy.

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    np, just checking in. Good luck with your things! :four_leaf_clover:

    点赞 评论 复制链接分享
  • weixin_39653733 weixin_39653733 4月前

    Overall I think this is a reasonable approach, let me know when you want me to do a complete review.

    点赞 评论 复制链接分享
  • weixin_39586353 weixin_39586353 4月前

    Regarding the signature, is there somethign wrong with doing:

     go
    func (client *Client) CaptureMessage(message string, contexts ...*Context) string
    

    That way you can utilize context/types/merging, and still keep the signature variadic. This will allow both the common case of:

     go
    client.CaptureMessage("lol broken")
    

    and

     go
    client.CaptureMessage("lol broken", &Context{Level: "error"})
    
    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    I like that. It's sort of a marriage of #14 and this one, and is sort of like how we do "optional interfaces" right now.

    点赞 评论 复制链接分享
  • weixin_39586353 weixin_39586353 4月前

    That's generally how the other clients work, and imo, it'd be awkward to need to define an "empty" object if you're not passing anything. The "extra" is just that. They're meant to be optional.

    So the normal case is typically: client.CaptureMessage("lol broken")

    And it'd be awkward to have to switch to a different method entirely to do something like: client.CaptureMessageWithContext("lol broken", &Context{...})

    And just as awkward would be: client.CaptureMessage("lol broken", &Context{})

    Just to satisfy the signature.

    This approach could also technically allow passing a bunch of contexts (for whatever reason) and merge them all together.

    So client.CaptureMessage("lol broken", &Context{...}, &Context{...}) would be a thing you could do.

    点赞 评论 复制链接分享
  • weixin_39575054 weixin_39575054 4月前

    Right on. :v: Fully with ya.

    点赞 评论 复制链接分享

相关推荐