neithere / argh

An argparse wrapper that doesn't make you say "argh" each time you deal with it.
http://argh.rtfd.org
GNU Lesser General Public License v3.0
369 stars 55 forks source link

TypeError: cannot picke '_io.TextIOWrapper' regression in 0.30 for `type=FileType(..), default=sys.stdin)` #204

Closed mfussenegger closed 10 months ago

mfussenegger commented 10 months ago

It looks like there was a regression in the assembling refactor. The following used to work:

import sys
import argh
from argparse import FileType

@argh.arg("-x", type=FileType("r", encoding="utf-8"), default=sys.stdin)
def foo(*, x):
    pass

if __name__ == "__main__":
    p = argh.ArghParser(prog="foo")
    p.add_commands([foo])
    p.dispatch()

Since 0.30.0 it results in the following stacktrace:

  File "/path/to/project/venv/lib/python3.11/site-packages/argh/helpers.py", line 47, in add_commands
    return add_commands(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 557, in add_commands
    set_default_command(
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 319, in set_default_command
    spec = _prepare_parser_add_argument_spec(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 354, in _prepare_parser_add_argument_spec
    spec = ParserAddArgumentSpec(**asdict(parser_add_argument_spec))
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1284, in asdict
    return _asdict_inner(obj, dict_factory)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1291, in _asdict_inner
    value = _asdict_inner(getattr(obj, f.name), dict_factory)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1325, in _asdict_inner
    return copy.deepcopy(obj)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/copy.py", line 161, in deepcopy
    rv = reductor(4)
         ^^^^^^^^^^^
TypeError: cannot pickle '_io.TextIOWrapper' object

I suspect it can't picke sys.stdin?


Side note: Was it intentional that the keyword-only arguments become already mandatory? https://github.com/neithere/argh/issues/191 mentioned a smooth transition path.

If you change the above example to:

@argh.arg("-x", type=FileType("r", encoding="utf-8"), default=sys.stdin)
def foo(x):
    pass

It fails with argh.exceptions.AssemblingError: foo: argument "x" declared as positional (in function signature) and optional (via decorator)

Not that big of a deal and easy enough to adjust, but given that the issue mentions that there should be a smooth migration I wanted to mention it too.

neithere commented 10 months ago

Hi @mfussenegger, thank you for the detailed report!

Confirming the regression. The fix is ready, to be released as 0.30.1 today or later this week.


Re not-so-smooth migration to the new name mapping policy: very good point, I've added a more informative error there with a hint to the possible solution.

FYI, I was indeed planning a smooth transition but the path with deprecation warnings would mean the necessity to update absolutely every app with an explicit policy and then deprecate the legacy policy. It would be far longer and require much more intervention from app developers. This way they have three options: a) stay at v0.29, b) upgrade but stay with the legacy policy, c) upgrade and add that asterisk.

Anyway, thanks for the valuable feedback, I was a bit nervous about this one when preparing the release 😄

mfussenegger commented 10 months ago

Re not-so-smooth migration to the new name mapping policy: very good point, I've added a more informative error there with a hint to the possible solution.

I think part of the problem was that I first looked at the Github release (https://github.com/neithere/argh/releases/tag/v0.30.0). That led me to https://github.com/neithere/argh/pull/199

If I had looked at the changes file it would've been much clearer. I thought maybe there was another issue preventing the smooth migration from working.