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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Jan 5, 2025
Merged

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

merged 10 commits into from
Jan 5, 2025

Conversation

sscherfke
Copy link
Contributor

@sscherfke sscherfke commented Nov 28, 2024 •
edited
Loading

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.

@sscherfke
Handle Logger.exception() outside "except" block
292e5bf 
Fixes: hynek#634
@sscherfke
Continue fixing
f9bdfc6
@sscherfke
Try a more radical approach to fix _figure_out_exc_info()
3644778 
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.

@sscherfke
Revert hynek#657
42314f6 
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 •
edited
Loading

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.

hynek
hynek requested changes Jan 4, 2025
View reviewed changes
Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

This very much needs a changelog entry – and as I asked inline, I wonder how breaking this would be considered given how many complains we got for the last release. 🤔

tests/test_tracebacks.py Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
tests/test_tracebacks.py Outdated Show resolved Hide resolved
@sscherfke
Copy link
Contributor Author

I'll add a changelog entry when the code is ready.

hynek
hynek approved these changes Jan 4, 2025
View reviewed changes
Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

ok changelog pls and let's get this behind us

@sscherfke
Add changelog entry
1efd6e8 
Also remove the entry for the now obsolete hynek#657
@sscherfke
Copy link
Contributor Author

Done, feel free to edit. :-)

@hynek hynek merged commit 2f7298a into hynek:main Jan 5, 2025
19 checks passed
@sscherfke sscherfke deleted the issue-634 branch January 5, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hynek hynek hynek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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