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

Handle Logger.exception() outside "except" block #680

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sscherfke
Copy link
Contributor

@sscherfke sscherfke commented Nov 28, 2024

Summary

Fixes #634, follow-up of #635

Pull Request Check List

  • Do not open pull requests from your main branch – use a separate branch!
    • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
    • This is not a pre-requisite for your pull request to be accepted, but you have been warned.
  • Added tests for changed code.
    • The CI fails with less than 100% coverage.
  • New APIs are added to our typing tests in api.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      • The next version is the second number in the current release + 1. The first number represents the current year. So if the current version on PyPI is 23.1.0, the next version is gonna be 23.2.0. If the next version is the first in the new year, it'll be 24.1.0.
  • Documentation in .rst and .md files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

The main problem of the function is that *v* can really be anything
(depending on what the users) do and the annotated return type did not
properly match it (e.g., if *v* was "0", the return value would be "0").

The function now rigorously checks the user input and either returns
the desired result (an exc info tuple) or None.  This makes it easier
and safer to use this function.
@hynek hynek changed the title Issue 634 Handle Logger.exception() outside "except" block Nov 29, 2024
@hynek
Copy link
Owner

hynek commented Nov 30, 2024

the tests are red 😇

also have you seen, that there's been changes to missing tracebacks lately – notably #657? 🤔 not sure if that maybe interacts somehow with your changes since it's thematically adjacent.

@sscherfke
Copy link
Contributor Author

Didn't have time yet to fix the tests. Just recreated the MR for now. #657 is superfluous now. The problem it handled should no longer be possible with this PR.

The changes introduced in hynek#657 are no longer needed since
_format_exception() is now only called if _figure_out_exc_info() detects
an actual exception
@sscherfke
Copy link
Contributor Author

sscherfke commented Dec 1, 2024

Checks are green now. You should still carefully review the changes. ;-)

@sscherfke
Copy link
Contributor Author

Do you need anything else from me for this PR to get merged? :-)

@hynek
Copy link
Owner

hynek commented Dec 15, 2024

Sorry for the delays, I’ll get to structlog (there‘s quite a bit of open issues around exceptions 😰) right after pushing out attrs. I think it’s ready.

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.

ExceptionRenderer w/ ExceptionDictTransformer unable to handle exception log outside exception handler
2 participants