Another way to try to be on spec! #19

taylortrimble · 2014-01-05T06:04:28Z

Remember #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`

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

Only allows tags and interfaces 😦

Bad Variadic Solution

func (client *Client) CaptureMessage(message string, attr ...interface{}) string

Not type safe like at all 😩

This Pull Request

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)! ☺️

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:

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 😉 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

Includes ability to merge event info. That’s pretty much it. A refactor thrown in while I was trying to make sense of life.

taylortrimble · 2014-01-06T01:43:13Z

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

taylortrimble · 2014-01-06T23:07:00Z

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

titanous · 2014-01-06T23:07:51Z

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

taylortrimble · 2014-01-06T23:09:15Z

@titanous np, just checking in. Good luck with your things! 🍀

titanous · 2014-01-12T16:36:31Z

client.go

 // when client is nil. A channel is provided if it is important to check for a
 // send's success.
-func (client *Client) Capture(packet *Packet, captureTags map[string]string) (eventID string, ch chan error) {
+func (client *Client) Capture(message string, eventInfo *EventInfo) (eventID string, ch chan error) {


What is the difference between Capture and CaptureMessage, their signatures are the same. I don't think Capture should take a message.

Does it make sense to call this Send? Should it be capturing anything? Isn't it what all of the Capture* methods use to actually send the packet after building it?

I added Capture for parity with the Python client. I messed up the signature badly though. Will change it and let you know.

It might also make sense to see if it just becomes Send.

Changed. This might be ready to take a look at.

titanous · 2014-01-12T16:42:22Z

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

titanous · 2014-01-12T16:44:48Z

client.go

-	if res.StatusCode != 200 {
-		return fmt.Errorf("raven: got http status %d", res.StatusCode)
+
+	// Merge interfaces together


Hmm, shouldn't all of this merging code be in event.go with the rest?

This was different because it was "defaults", but this is done differently now and is more obvious. thank you for catching that!

mattrobenolt · 2014-02-05T09:00:49Z

Regarding the signature, is there somethign wrong with doing:

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:

client.CaptureMessage("lol broken")

and

client.CaptureMessage("lol broken", &Context{Level: "error"})

taylortrimble · 2014-02-05T09:01:58Z

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.

mattrobenolt · 2014-02-05T09:05:29Z

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.

taylortrimble · 2014-02-05T09:07:02Z

Right on. ✌️ Fully with ya.

titanous · 2014-02-05T15:43:16Z

+1, I like this approach.

mattrobenolt · 2014-02-16T06:22:36Z

Anything I can do to help with this?

taylortrimble · 2014-12-11T23:40:35Z

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!

Also initialize events in *Event method Also rename EventInfo to Event throughout.

Need some tests for merging. I keep getting them backwards and that needs to break shit.

taylortrimble · 2014-12-12T05:36:53Z

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

Conflicts: client.go client_test.go Had to manually move some code that got copied into new files. Hopefully I got everything! Here’s what changed in master since I started: https://github.com/getsentry/raven-go/compare/30337dc77126a66b25ac836918 f31b63f10f969a...master

Remove CheckError example: need to replace with better example that makes better use of CaptureError and uses a persistent client.

taylortrimble · 2014-12-15T02:11:03Z

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. 😄

taylortrimble · 2014-12-16T06:21:43Z

@mattrobenolt's awesome API is now implemented! 💃

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!

Dominantly client.go

Fix tests

MrSaints · 2016-05-02T00:10:53Z

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

taylortrimble added 2 commits January 4, 2014 14:16

Rename packet to event info

46a0515

contextual EventInfo can be passed to capture methods

893086b

Includes ability to merge event info. That’s pretty much it. A refactor thrown in while I was trying to make sense of life.

taylortrimble mentioned this pull request Jan 5, 2014

UUID should not have hyphens #20

Closed

taylortrimble added 2 commits January 5, 2014 12:10

Reinstate FormatUUID as FormatEventID

2aa45f6

Merge interfaces too

88aef50

titanous reviewed Jan 12, 2014
View reviewed changes

taylortrimble mentioned this pull request Feb 5, 2014

Move to getsentry org? #23

Closed

taylortrimble mentioned this pull request Feb 5, 2014

Remove unused FormatUUID #24

Closed

Add events to queue with host and auth

5742508

Also initialize events in *Event method Also rename EventInfo to Event throughout.

taylortrimble mentioned this pull request Dec 12, 2014

Breaking API, Versioning, and Beyond #43

Closed

Capture method just takes event. Update event merges.

504e0e4

Need some tests for merging. I keep getting them backwards and that needs to break shit.

taylortrimble mentioned this pull request Dec 12, 2014

Call func wrapped by CapturePanic when client nil #41

Closed

taylortrimble added 2 commits December 14, 2014 18:06

Fix examples from merge from master

ed49498

Remove CheckError example: need to replace with better example that makes better use of CaptureError and uses a persistent client.

Whoops getsentry

30aad81

taylortrimble added 3 commits December 15, 2014 21:54

Alias Event to Context where appropriate.

9df36f8

Use variadic capture contexts

07b28e2

Whoops I am dumb.

f8d3109

taylortrimble added 8 commits December 19, 2014 21:38

Documentation and Organization

5cd4f8f

Dominantly client.go

Why didn't this get committed?

41a9f20

Reorganize and privatize methods for docs

618c454

Fix tests

Remove guard, fix tests instead.

3a1c612

Typo

c07a968

Make JSON on event private

599f3dc

Add a bunch of documentation.

fd48cea

GitHub app commit things _please_

9455564

taylortrimble mentioned this pull request Dec 30, 2014

added the ability to provide optional extra data to captured errors and messages #45

Closed

taylortrimble closed this Apr 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another way to try to be on spec! #19

Another way to try to be on spec! #19

taylortrimble commented Jan 5, 2014

taylortrimble commented Jan 6, 2014

taylortrimble commented Jan 6, 2014

titanous commented Jan 6, 2014

taylortrimble commented Jan 6, 2014

titanous Jan 12, 2014

titanous Jan 12, 2014

taylortrimble Dec 11, 2014

taylortrimble Dec 12, 2014

titanous commented Jan 12, 2014

titanous Jan 12, 2014

taylortrimble Dec 12, 2014

mattrobenolt commented Feb 5, 2014

taylortrimble commented Feb 5, 2014

mattrobenolt commented Feb 5, 2014

taylortrimble commented Feb 5, 2014

titanous commented Feb 5, 2014

mattrobenolt commented Feb 16, 2014

taylortrimble commented Dec 11, 2014

taylortrimble commented Dec 12, 2014

taylortrimble commented Dec 15, 2014

taylortrimble commented Dec 16, 2014

MrSaints commented May 2, 2016 •

edited

Loading

Another way to try to be on spec! #19

Another way to try to be on spec! #19

Conversation

taylortrimble commented Jan 5, 2014

Current master

Bad Variadic Solution

This Pull Request

What's Next

taylortrimble commented Jan 6, 2014

taylortrimble commented Jan 6, 2014

titanous commented Jan 6, 2014

taylortrimble commented Jan 6, 2014

titanous Jan 12, 2014

Choose a reason for hiding this comment

titanous Jan 12, 2014

Choose a reason for hiding this comment

taylortrimble Dec 11, 2014

Choose a reason for hiding this comment

taylortrimble Dec 12, 2014

Choose a reason for hiding this comment

titanous commented Jan 12, 2014

titanous Jan 12, 2014

Choose a reason for hiding this comment

taylortrimble Dec 12, 2014

Choose a reason for hiding this comment

mattrobenolt commented Feb 5, 2014

taylortrimble commented Feb 5, 2014

mattrobenolt commented Feb 5, 2014

taylortrimble commented Feb 5, 2014

titanous commented Feb 5, 2014

mattrobenolt commented Feb 16, 2014

taylortrimble commented Dec 11, 2014

taylortrimble commented Dec 12, 2014

taylortrimble commented Dec 15, 2014

taylortrimble commented Dec 16, 2014

MrSaints commented May 2, 2016 • edited Loading

Current `master`

MrSaints commented May 2, 2016 •

edited

Loading