New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit support for sending string noticies #104

Merged
merged 1 commit into from Oct 25, 2017

Conversation

Projects
None yet
3 participants
@sorentwo
Contributor

sorentwo commented Oct 18, 2017

While it was possible to send an arbitrary notice by constructing an error manually, it wasn't possible to pass a string to the notify function. Sending a notice directly from a string is more convenient, and it make the exact arguments that the notify function accepts more obvious.

This refactors the Honeybadger.Notice module a little to guard the accepted types a bit more. Part of the guarding process involved adding typespecs for the components that make up a Honeybadger.Notice struct.

Addresses #92

@joshuap This is a little yak shaving to prepare Honeybadger.notify/3 to handle arbitrary error logging with iodata.

@joshuap

It sounds and looks great to me. I asked a few questions. @minhajuddin do you mind also reviewing?

Show outdated Hide outdated lib/honeybadger.ex
%{backtrace: [], class: "RuntimeError", message: "oops", tags: []}
"""
@spec new(noticeable, Map.t, list) :: t
def new(error, metadata, backtrace)

This comment has been minimized.

@joshuap

joshuap Oct 24, 2017

Member

Do you mind explaining how this line works? I can't remember what Elixir does when there is no function body.

@joshuap

joshuap Oct 24, 2017

Member

Do you mind explaining how this line works? I can't remember what Elixir does when there is no function body.

This comment has been minimized.

@sorentwo

sorentwo Oct 24, 2017

Contributor

It defines a function head without any body. Usually you do this to specify defaults for functions with multiple clauses. In this case I did it for the @spec above it. Functions can only have one spec, regardless of how many clauses there are.

@sorentwo

sorentwo Oct 24, 2017

Contributor

It defines a function head without any body. Usually you do this to specify defaults for functions with multiple clauses. In this case I did it for the @spec above it. Functions can only have one spec, regardless of how many clauses there are.

@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Oct 24, 2017

Member

@sorentwo btw, any idea why the build is failing?

Member

joshuap commented Oct 24, 2017

@sorentwo btw, any idea why the build is failing?

@joshuap joshuap requested a review from minhajuddin Oct 24, 2017

@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 24, 2017

Contributor

@joshuap I took a look at the run time of that job and it is a full 2-3 seconds longer than others. The failures were from message assertion timeouts, so I think things were running slowly. It is a shared environment, so possibly some noisy neighbors. Re-running the build should fix it.

In a related note, sorry I didn't notice this before. I don't get any notifications from Travis when the build breaks.

Contributor

sorentwo commented Oct 24, 2017

@joshuap I took a look at the run time of that job and it is a full 2-3 seconds longer than others. The failures were from message assertion timeouts, so I think things were running slowly. It is a shared environment, so possibly some noisy neighbors. Re-running the build should fix it.

In a related note, sorry I didn't notice this before. I don't get any notifications from Travis when the build breaks.

@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Oct 24, 2017

Member

No worries. :) I tried assigning you to the PR, not sure if that will make you get notifications.

Hrmm, I'm not sure why but that build keeps failing (2 attempts so far). I can wait a while and try again.

Member

joshuap commented Oct 24, 2017

No worries. :) I tried assigning you to the PR, not sure if that will make you get notifications.

Hrmm, I'm not sure why but that build keeps failing (2 attempts so far). I can wait a while and try again.

@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 24, 2017

Contributor

Thanks for trying the rebuild. I tried it locally and got the same error. It turns out that the Erlang version of the argument error is simply :badarg, which is an atom. The Notice.new function allowed it, but Honeybadger.notify didn't.

I've pushed up a fix (and an explicit test). Glad the tests caught it.

Contributor

sorentwo commented Oct 24, 2017

Thanks for trying the rebuild. I tried it locally and got the same error. It turns out that the Erlang version of the argument error is simply :badarg, which is an atom. The Notice.new function allowed it, but Honeybadger.notify didn't.

I've pushed up a fix (and an explicit test). Glad the tests caught it.

Show outdated Hide outdated lib/honeybadger/notice.ex
Show outdated Hide outdated lib/honeybadger.ex
Add explicit support for sending string noticies
While it was possible to send an arbitrary notice by constructing an
error manually, it wasn't possible to pass a string to the `notify`
function. Sending a notice directly from a string is more convenient,
and it make the exact arguments that the `notify` function accepts more
obvious.

This refactors the `Honeybadger.Notice` module a little to guard the
accepted types a bit more. Part of the guarding process involved adding
typespecs for the components that make up a `Honeybadger.Notice` struct.

Addresses #92

@joshuap joshuap merged commit 3c90124 into honeybadger-io:master Oct 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment