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 HTTPError response text #1008

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

dases
Copy link
Contributor

@dases dases commented Jan 24, 2025

Proposed changes

Logging the response text for an HTTPError greatly improves the developer's ability to figure out what might going wrong with an integration.

Checklist

@@ -225,6 +225,8 @@ def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except requests.HTTPError as err:
social_logger.error(err.response.text)
Copy link
Member

Choose a reason for hiding this comment

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

Logging just the text response might be confusing, it can also be empty.

Suggested change
social_logger.error(err.response.text)
social_logger.error("request failes with %d: %s", err.response.status_code, err.response.text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, better. Thanks.

@dases dases force-pushed the feat/log_http_error_response branch from acad3c4 to 26bec93 Compare January 24, 2025 08:28
@dases
Copy link
Contributor Author

dases commented Jan 24, 2025

Seems to be a clash of lint constraints.
pre-commit.ci rewrites as f-string, which then fails check

@nijel
Copy link
Member

nijel commented Jan 24, 2025

It should use interpolation done by logging, what was reason for 6c3ea1a?

c0a5074 passed CI checks and could be merged.

@dases dases force-pushed the feat/log_http_error_response branch from fa8bb3c to c0a5074 Compare January 24, 2025 18:33
@dases
Copy link
Contributor Author

dases commented Jan 24, 2025

It should use interpolation done by logging, what was reason for 6c3ea1a?

c0a5074 passed CI checks and could be merged.

You're quite right. It looked wrong and I "corrected" it to interpolate a tuple of values like you would for print or whatever, but I see that's not required in logging calls. 🤦

Reverted to c0a5074

Thanks for feedback!

@nijel nijel enabled auto-merge (squash) January 27, 2025 13:18
@nijel nijel self-assigned this Jan 27, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.95%. Comparing base (326d4b3) to head (c0a5074).
Report is 65 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1008      +/-   ##
==========================================
+ Coverage   77.88%   77.95%   +0.06%     
==========================================
  Files         347      347              
  Lines       10669    10694      +25     
  Branches      504      455      -49     
==========================================
+ Hits         8310     8336      +26     
+ Misses       2200     2198       -2     
- Partials      159      160       +1     
Flag Coverage Δ
unittests 77.95% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nijel nijel merged commit 67abf8f into python-social-auth:master Jan 27, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants