FIX: fixed cleanup code in @contextlib.contextmanager
#340
Conversation
line_profiler/autoprofile/autoprofile.py::run()
kernprof.py::main()
tests/test_line_profiler.py::check_timings()
Fixed bug in `@contextlib.contextmanager`s where the cleanup code is
not called if an error occurred inside the `with` block
|
@Erotemic looks like your initial hunch was right on the money, I did mess the context managers up... |
TTsangSC
changed the title
@contextlib.contextmanager fix@contextlib.contextmanager
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
==========================================
+ Coverage 63.63% 63.78% +0.14%
==========================================
Files 13 13
Lines 1034 1041 +7
Branches 229 228 -1
==========================================
+ Hits 658 664 +6
- Misses 315 316 +1
Partials 61 61
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Can we just change these to class based context managers with |
|
Will do. IMO class-based CMs may add a bit of boilerplate, but maybe the pros (clarity) outweigh the cons here, considering that I just footgunned myself here with the alternative... anyway, will push in a moment, and then we can review which is better for the codebase. |
kernprof.py::_restore_list()
line_profiler/autoprofile/autoprofile.py::run().<locals>.restore_dict
tests/test_line_profiler.py::check_timings
Refactored to use classes instead of `@contextlib.contextmanager`
tests/test_kernprof.py::test_kernprof_sys_restoration()
Extended to also check for the restoration of `sys.modules`
tests/test_line_profiler.py::check_timings
Fixed on-exit assertion, which was always true regardless of the
timings because it was written `assert (cond, msg)` instead of
`assert cond, (msg)`
Co-authored-by: Jon Crall <erotemic@gmail.com>
|
Somehow CI keeps failing on MacOS with error 429 (b8dab90: https://github.com/pyutils/line_profiler/actions/runs/14977439218/job/42074090570, 7988790: https://github.com/pyutils/line_profiler/actions/runs/14977545152/job/42074530015)... seems to be a rate-limiting issue, probably because I pushed the two commits in quick succession... |
|
Reran the CI, and it worked. Everything else looks good. Merging. |
|
Cheers! |
tests/test_explicit_profile.py::enter_tmpdir
Now explicitly defined as a class, instead of using
`@contextlib.contextmanager` (see pyutils#340)
tests/test_explicit_profile.py::enter_tmpdir
Now explicitly defined as a class, instead of using
`@contextlib.contextmanager` (see pyutils#340)
tests/test_explicit_profile.py::enter_tmpdir
Now explicitly defined as a class, instead of using
`@contextlib.contextmanager` (see pyutils#340)
2 participants
Just noticed that I made the same mistake in #323, #332, and #339, where
@contextlib.contextmanageris used to wrap generator functions to construct context managers: theyieldstatement and the subsequent cleanup code wasn't wrapped in atry–finallyblock, so the cleanup is eschewed if any exception occurred inside thewithblock/decorated function.This PR fixes the bug in three functions:
kernprof.py::_restore_list()(used bymain())line_profiler/autoprofile/autoprofile.py::run.<locals>.restore_dict()tests/test_line_profiler.py::check_timings()One test is added to verify that the old behavior is problematic and is fixed by the PR:
tests/test_kernprof.py::test_kernprof_sys_restoration()Sorry for any inconvenience caused.