Repository navigation
fix(units): restore registry when add-enabled-units raises - #20795
Sudhakar7J wants to merge 1 commit into
Conversation
Each public units context factory pushed a new registry via _UnitContext and then mutated it. If validation raised (e.g. a name clash), the caller never received the context manager, so the pushed registry was never popped. A nested failure therefore corrupted the registry stack: the enclosing with-block popped the wrong level on exit and its custom definitions stayed enabled (astropygh-20790). Wrap the mutation in each of the six factories (add/set_enabled_units, add/set_enabled_equivalencies, add/set_enabled_aliases) with try/except BaseException that unwinds the pushed context before re-raising. Closes astropy#20790 Signed-off-by: Sudhakar <[email protected]>
|
Hi 👋 and thank you for your contribution! 🙏 This pull request is being closed automatically - but don't worry, we may re-open it if you carefully follow these instructions. Join the astropy slack using this invite link, head over to the #new-contributor-requests channel, and introduce yourself with a post that includes:
We strongly encourage contributions from people that use astropy or actively develop related scientific Python packages. We look forward to hearing from you and growing the Astropy community! Notes for maintainersIf you know this is a genuine user and the PR is is compliance with the AI policy, you may re-open this straight away. After two successful merged pull requests, the user will be added to the allowlist and future PRs will no longer be closed automatically. This user has not made any pull requests to this repository prior to this one. In addition, here are some statistics on the user's activity on GitHub:
|
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
Description
This pull request is to address #20790 where
add_enabled_unitscan leave a unit-registry context active when enabling a conflicting alias raises.The public factories constructed/pushed
_UnitContextbefore validation, so aValueErrorleaked the push — the enclosing context then popped the inner level on exit, leaving the custom definition enabled.Fix: each of the six factories (
add/set_enabled_units,add/set_enabled_equivalencies,add/set_enabled_aliases) now restores the registry onBaseExceptionby unwinding its own context before re-raising. Deferred construction was avoided to preserve the documented permanent (no-with) mode.Testing
test_enabled_units_context_restored_on_failed_add(full kcal issue repro, registry identity + restoration) andtest_enabled_unit_factories_restore_registry_on_error(other five factories)pytest --pyargs astropy.units.tests.test_units -k unit-> 390 passed, 2 skippedpytest --pyargs astropy.units.tests.test_units astropy.units.tests.test_equivalencies-> 470 passed, 2 skippedFixes #20790
AI Disclosure
Used Crouton Alpha (opencode/crouton-alpha-free) via OpenCode as a coding assistant to implement the fix and regression tests under the astropy AI policy (https://github.com/astropy/astropy-project/blob/main/policies/ai-policy.md). AI generated the initial patch and test cases; human reviewed the diff, verified behavior on astropy 8.0.2, and ran the full test suite.