Skip to content
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

Log exceptions too when logging errors #1918

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Conversation

andreak
Copy link
Member

@andreak andreak commented Sep 15, 2017

No description provided.

@andreak andreak added this to the 3.2.0-M2 milestone Sep 15, 2017
Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 TY

If you won't have time to make these changes today let me know and I'm more than happy to make them.

logger.error("'embed' snippet failed with message: "+msg, e)
case _ =>
logger.error("'embed' snippet failed with message: "+msg)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably would have represented this as:

case Failure(msg, Full(e), _) => logger.error("'embed' snippet failed with message: "+msg, e)
case Faulure(msg, _, _) => logger.error("'embed' snippet failed with message: "+msg)

case Full(e) => logger.error("Failed to deserialize JSON message "+p+". Error "+msg, e)
case _ => logger.error("Failed to deserialize JSON message "+p+". Error "+msg)
}
p
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here to unifying the case style.

@farmdawgnation farmdawgnation merged commit aa24c2d into master Sep 15, 2017
@farmdawgnation farmdawgnation deleted the ajk-log-exceptions branch September 15, 2017 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants