Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

Refactor capture helpers #10

Merged
merged 1 commit into from
Dec 30, 2013
Merged

Refactor capture helpers #10

merged 1 commit into from
Dec 30, 2013

Conversation

titanous
Copy link
Contributor

What do you think @tylrtrmbl?

@taylortrimble
Copy link
Contributor

I like the renames.

I also like the flexibility of adding interfaces to the Capture* methods, but think we shouldn't; there's a precedent out there right now from the other clients that captureMessage and captureException function sort of like "just capture a message" and "just capture an exception" (or error, in our case). There's usually a capture method that gets all the bells and whistles instead.

So I'd say keep the renames and keep the rest as is. We could add interfaces ...Interface to capture, but there's not a big reason to since we're formatting a full packet anyway.

Brilliant command of the language, by the way: append(interfaces, &Message{message, nil})...

@titanous
Copy link
Contributor Author

This only adds complexity to the signature, there is no requirement that any interfaces are passed in: http://play.golang.org/p/9WjDUOnMF_

@taylortrimble
Copy link
Contributor

I'm still not totally for changing the signature - it looks to be spec'd by the client docs:

Additionally, you should provide methods (depending on the platform) which allow for capturing of a basic message and an exception-type:

RavenClient::captureMessage(string $message)
RavenClient::captureException(exception $exception)

The above methods should also allow optional arguments (or a map of arguments). For example:

client.captureException(myException, {
    'tags': {'foo': 'bar'},
})

and the other clients seem to stick to that pretty closely. I'll leave the final call to you though.

@taylortrimble
Copy link
Contributor

On second thought, I really do like that flexibility and we can demonstrate in the README it works as expected (like the clients + docs would leave you to believe).

👍

@taylortrimble
Copy link
Contributor

Haha. We're on top of one another. :) Still final call to you then. 😜

@titanous
Copy link
Contributor Author

Cool, re-added it. I also rearranged the arguments of CapturePanic to put f first for consistency.

@taylortrimble
Copy link
Contributor

:squirrel:

titanous added a commit that referenced this pull request Dec 30, 2013
@titanous titanous merged commit 58f427c into master Dec 30, 2013
@titanous titanous deleted the refactor-helpers branch December 30, 2013 00:02
mzuneska pushed a commit to mzuneska/raven-go that referenced this pull request Aug 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants