Fix Argument.__repr__ crash when _action is not initialized#14429
Fix Argument.__repr__ crash when _action is not initialized#14429EternalRights wants to merge 5 commits intopytest-dev:mainfrom
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
EternalRights
force-pushed
the
fix-argparse-attrerror
branch
from
4f863ed to
4cd4ca7
Compare
for more information, see https://pre-commit.ci
| def test_argument_repr_initialized(parser: parseopt.Parser) -> None: | ||
| """Argument.__repr__ works normally when properly initialized.""" | ||
| parser.addoption("--myflag", dest="myflag", help="test flag") | ||
| option = parser._anonymous.options[-1] | ||
| result = repr(option) | ||
| assert "opts:" in result | ||
| assert "dest:" in result | ||
|
|
||
|
|
||
| def test_argument_repr_with_type(parser: parseopt.Parser) -> None: | ||
| """Argument.__repr__ includes type when set.""" | ||
| parser.addoption("--count", type=int, dest="count", help="count") | ||
| option = parser._anonymous.options[-1] | ||
| result = repr(option) | ||
| assert "type:" in result | ||
| assert "int" in result |
There was a problem hiding this comment.
Why two separate tests for this? Wouldn't make sense to also test the type in test_argument_repr_initialized? For that matter, test_argument_repr_initialized could check the entire string, instead of relying on in checks for a few keywords.
There was a problem hiding this comment.
Thanks for the review! You're right on all points.
-
I kept
test_argument_repr_uninitializedseparate because it tests a fundamentally different construction path (bare__new__vs proper initialization throughparser.addoption). This follows the same pattern astest_saferepr.pywhere error paths are separate from normal ones. However, I agree theinitializedandwith_typecases should be merged as they test the same construction path. -
Agreed — the
typebranch should be tested withintest_argument_repr_initialized, not as a separate test. I'll merge them. -
Agreed — I'll switch to exact string matching with
==. I seetest_saferepr.pyconsistently uses exact comparison.
I'll push the updates shortly.
… string matching Per nicoddemus review feedback: - Merge test_argument_repr_with_type into test_argument_repr_initialized as they test the same construction path - Switch from 'in' checks to exact == string matching - Cover both 'without type' and 'with type' branches in one test
for more information, see https://pre-commit.ci
2 participants
Ran into this while looking at #13817. When
Argument.__init__doesn't complete (e.g. ifargparse.Actionraises during construction), callingrepr()on the partially-initializedArgumenttriggers a secondaryAttributeErrorbecauseself._actiondoesn't exist yet. This masks the original error message.The fix uses
getattr(self, "_action", None)in__repr__and returns"Argument(<uninitialized>)"when_actionis missing, as RonnyPfannschmidt suggested. This way the original exception message gets through instead of being buried under a secondary crash.