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

make report_exc_info to accept level arg like report_message #22

Merged
merged 2 commits into from May 9, 2014
Merged

make report_exc_info to accept level arg like report_message #22

merged 2 commits into from May 9, 2014

Conversation

jamesonjlee
Copy link
Contributor

I didn't realize that report_exc_info didn't actually accept/do anything with level='info' like report_message,
so I added it, and wrote the test.

@@ -195,7 +195,7 @@ def report_exc_info(exc_info=None, request=None, extra_data=None, payload_data=N
exc_info = sys.exc_info()

try:
return _report_exc_info(exc_info, request, extra_data, payload_data)
return _report_exc_info(exc_info, request, extra_data, payload_data, level=level)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of modifying the _report_exc_info() signature, I think it would be better to update payload_data in here like so:

if level:
    payload_data = payload_data or {}
    payload_data['level'] = level

try:
    return _report_exc_info(exc_info, request, extra_data, payload_data)
except:
    # ...

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I purposely did this to follow report_message's style (where if payload_data['level'] is provided it will override provided level arg). So the order is exception level -> level arg -> payload_data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point!

@sbezboro
Copy link
Contributor

sbezboro commented May 9, 2014

Thanks for the PR! Just made a small comment, let me know what you think

@sbezboro sbezboro merged commit ff58174 into rollbar:master May 9, 2014
@sbezboro
Copy link
Contributor

sbezboro commented May 9, 2014

Merged and pushed to PyPI as version 0.7.4

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.

3 participants