Raise exceptions directly instead of logging-and-raising - #58
Merged
Conversation
Replace the 40 logger.error("msg", exception=ExcType) call sites with plain
raise ExcType("msg"). The custom logger raised the exception as a side effect of
logging, which (a) reads as logging when it is really control flow, (b) depends
on which package last won logging.setLoggerClass, and (c) hid the control flow
from pylint (every such function looked like it could fall through). Exception
types and messages are unchanged, so the pytest.raises assertions are preserved.
Also fixes a latent bug in dimensionality(): a 0D branch built a ValueError
without raising it (and fell through returning None); it now raises.
Clears inconsistent-return-statements, no-else-raise and raise-missing-from;
pylint 8.79 -> 8.93. No behaviour change (full suite green with binaries).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
==========================================
+ Coverage 95.25% 95.93% +0.68%
==========================================
Files 22 22
Lines 1286 1280 -6
==========================================
+ Hits 1225 1228 +3
+ Misses 61 52 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add tests for the reachable Thermo.__init__ guards (unsupported engine, negative temperature, negative pressure). Mark as no-cover the guards that are unreachable while beartype enforces parameter types (the None checks and the dead _transform_units branch) and the unreachable 0D branch in dimensionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the package's log-and-raise control-flow pattern with explicit
raise, removing a real footgun.Problem
40 call sites did
self.logger.error("msg", exception=TSValueError), where the custom logger raises the exception as a side effect of logging. That:logging.setLoggerClass(PQAnalysis'sCustomLoggeralso raises on.error, so the behaviour is non-obvious);inconsistent-return-statements).I hit this twice while building screening (had to log failures at
warning, and a%-format crash).Change
self.logger.error("msg", exception=ExcType)->raise ExcType("msg")across api.py, system.py, thermo.py, atoms.py, inputFileReader.py (40 sites, net -121 lines).pytest.raises(..., match=...)/assert str(e.value) == ...still holds.except:lookups nowraise ... from None(no misleading chain).dimensionality(): a 0D branch constructed aValueErrorwithout raising it (silently returnedNone); it now raises.Result
inconsistent-return-statements(13 -> 0), plus theno-else-raise/raise-missing-fromthe change would otherwise introduce.Note
This removes ThermoScreening's dependence on log-and-raise. The underlying logger machinery (the
exception=param incustom_logging.py) is left in place but now unused by the package;screening.pystill logs failures atwarningbecause the active logger still raises on.error. Fully untangling the dualCustomLoggeris a separate, larger change.