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
New major version of notifier #47
Conversation
… challenge accepted. |
FYI a couple of |
And build files within |
} | ||
// Don't notify if this error class should be ignored | ||
if (config.shouldIgnoreClass(report.getExceptionName())) { | ||
logger.debug("Error not reported to Bugsnag - exception class is in 'ignoreClasses'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should log the class name in this message, for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a style decision? Adding simple getter/setter functions increases the size of the jar, which admittedly is not a major concern, but not sure of the benefits.
@loopj It was just a best practice/style decision, but I've reverted that commit. |
To get the build to work in all the JDKs I have:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Green tests 🎉
* @return the exceptions that make up the error. | ||
*/ | ||
@Expose | ||
public List<Exception> getExceptions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This either needs to be protected
or Exception
needs to be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, I don't think it needs to be public.
} | ||
|
||
@Expose | ||
public List<ThreadState> getThreads() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This either needs to be protected
or ThreadState
needs to be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, I don't think it needs to be public.
* @param config the configuration for the report. | ||
* @param throwable the error to create the report for. | ||
*/ | ||
public Report(Configuration config, Throwable throwable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the expectation is that this always get built from Bugsnag.buildReport we should remove the public
here to make this package protected. This will enforce the API usage which will make it easier to maintain and refactor in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, I don't think it needs to be public.
* {@link Severity#WARNING} or {@link Severity#INFO} | ||
* @return true unless the error report was ignored | ||
*/ | ||
public boolean notify(Throwable throwable, Severity severity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a notify(Throwable, Severity, Callback)
for completeness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add this yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
JSONUtils.safePut(tab, entry.getKey(), entry.getValue()); | ||
} | ||
} else { | ||
addToTab("Custom Data", tabName, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we officially dropping support for adding data without specifying a tab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, it seems best to force choosing a tab name.
* @param endpoint the endpoint to send reports to | ||
* @see #setDelivery | ||
*/ | ||
public void setEndpoint(String endpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now need to declare the protocol explicitly i.e. can't use localhost
have to use http://localhost
. We should document this in the migration instructions.
Omg |
ChangeLog
org.slf4j.Logger
Severity
is now an enum instead of aString
Client
object has been renamed toBugsnag
Event
object has been renamed toReport
Report
object is now exposed for ease of attaching diagnostics to error reportsReport
methodsBugsnag.notify
callsDelivery
interface