Make project compatible with Python 3.7+ #37

kytta · 2023-01-07T00:26:58Z

Closes #35

kytta · 2023-01-07T00:28:03Z

So far, it's a lot of find-and-replace work. I have replaced the types, but get_origin() needs to be replaced, too. As of now, it's 447 tests failed, 12 tests passed :)

On error, the older versions of argparse call the error() method, which is already overridden in a way that we like. So, to fix the tests, we just do not provide the parameter to the constructor on older Python versions.

kytta · 2023-01-07T13:42:37Z

Alright, I have resolved almost everything that needed to be resolved. There is one last problem, though.

The test argparse.test_parser.py::test_invalid_arguments fails when a subparser doesn't have a dest. It happens right on the last step, where the error message gets generated. This was a known bug (see python/cpython#73484) which was fixed in 3.11 (python/cpython#3680) and backported to 3.9 (python/cpython#27304). Since it's a module function, we can't reliably backport/patch it.

@SupImDos any ideas what we can do here? I propose adding a default metavar to the SubParsersAction with the value of, e.g., "COMMAND":

class SubParsersAction(argparse._SubParsersAction):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault("metavar", "COMMAND")
+        super().__init__(*args, **kwargs)

Basically, this would be analogue to this:

parser = argparse.ArgumentParser()
subparsers = parser.add_subparsers(metavar="COMMAND", required=True)
test_sub = subparsers.add_parser("test")
test_sub.add_argument("--flag", type=bool, default=False)

$ python3.7 parsing.py --help
usage: parsing.py [-h] COMMAND ...

positional arguments:
  COMMAND

optional arguments:
  -h, --help  show this help message and exit

$ python3.7 parsing.py       
usage: parsing.py [-h] COMMAND ...
parsing.py: error: the following arguments are required: COMMAND

The same code under Python 3.7, but without metavar

$ python3.7 parsing.py --help
usage: parsing.py [-h] {test} ...

positional arguments:
  {test}

optional arguments:
  -h, --help  show this help message and exit

$ python3.7 parsing.py       
Traceback (most recent call last):
  File "parsing.py", line 9, in <module>
    parser.parse_args()
  File "/Users/nikita/.pyenv/versions/3.7.16/lib/python3.7/argparse.py", line 1755, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/Users/nikita/.pyenv/versions/3.7.16/lib/python3.7/argparse.py", line 1787, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/Users/nikita/.pyenv/versions/3.7.16/lib/python3.7/argparse.py", line 2022, in _parse_known_args
    ', '.join(required_actions))
TypeError: sequence item 0: expected str instance, NoneType found

SupImDos · 2023-01-09T08:10:01Z

@kytta
Wow, thanks so much for all your hard work. This looks great!

1. Minor Type Checking Error

Looks like we might need to add a type: ignore[union-attr] to the BooleanOptionalAction backport so the CI/CD passes. The Python standard library typing is always a bit dodgy.

https://github.com/kytta/pydantic-argparse/blob/python-37/pydantic_argparse/argparse/actions.py#L194

-            setattr(namespace, self.dest, not option_string.startswith('--no-'))
+            setattr(namespace, self.dest, not option_string.startswith('--no-'))  # type: ignore[union-attr]

2. Argparse SubParser Backport Bug

Hmm this is an interesting one. Ideally I would like to keep the same behaviour (i.e., show the different command options across the different versions. i.e.,

$ python examples/commands.py
usage: Example Program [-h] [-v] [--verbose] {build,serve} ...
Example Program: error: the following arguments are required: {build,serve}

I can think of a few things we can do:

Default Static Metavar
As you suggest above, we can provide a default/dummy metavar to the SubParsersAction.

pydantic_argparse/argparse/actions.py

class SubParsersAction(argparse._SubParsersAction):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault("metavar", "COMMAND")
+        super().__init__(*args, **kwargs)

However, this means the help does not display the actual options we have (i.e., in this case {build,serve} - just the COMMAND string.

$ python examples/commands.py
usage: Example Program [-h] [-v] [--verbose] COMMAND ...
Example Program: error: the following arguments are required: COMMAND

Dynamic Metavar Using Property
Another option we have is to hijack the metavar value using a @property. This way we can dynamically construct the string with self.choices to provide the desired {build,serve} string. It is a bit messy though, and we have to provide a dummy @metavar.setter method. Not sure if I really like this option.

pydantic_argparse/argparse/actions.py

class SubParsersAction(argparse._SubParsersAction):
+   @property
+   def metavar(self) -> Optional[str]:
+       """Docstring...
+
+       Returns:
+           Optional[str]: ...
+       """
+       # Explanation?
+       # Type checking?
+       # What if no `self.choices`?
+       return '{' + ','.join(self.choices) + '}'
+
+   @metavar.setter
+   def metavar(self, value: Any) -> None:
+       """Docstring...
+
+       Args:
+           value (Any): ...
+       """
+       # Dummy method, we can't allow it to be set
+       return None

The desired result is produced below.

$ python examples/commands.py
usage: Example Program [-h] [-v] [--verbose] {build,serve} ...
Example Program: error: the following arguments are required: {build,serve}

Backport and MonkeyPatch _get_action_name
Finally, we could provide our own monkey-patch for the fixed _get_action_name method as below. This would provide the desired functionality and be relatively clean? I think I like this, but I'm not sure if we might introduce some issues by monkey-patching the standard library.

pydantic_argparse/argparse/patches.py

"""Docstring..."""


# Standard
import argparse
import sys

# Typing
from typing import Optional


# Some comments here explaining why we do this
# https://github.com/python/cpython/blob/main/Lib/argparse.py#L739-L751
if sys.version_info < (3, 9):
    def _get_action_name(argument: Optional[argparse.Action]) -> Optional[str]:
        """Docstring."""
        if argument is None:
            return None
        elif argument.option_strings:
            return '/'.join(argument.option_strings)
        elif argument.metavar not in (None, argparse.SUPPRESS):
            return argument.metavar  # type: ignore[return-value]
        elif argument.dest not in (None, argparse.SUPPRESS):
            return argument.dest
        elif argument.choices:
            return '{' + ','.join(argument.choices) + '}'
        else:
            return None

    # Monkey-Patch
    argparse._get_action_name = _get_action_name

The desired result is produced below.

$ python examples/commands.py
usage: Example Program [-h] [-v] [--verbose] {build,serve} ...
Example Program: error: the following arguments are required: {build,serve}

@kytta I think at the moment I prefer (3). What do you think?

kytta · 2023-01-09T09:11:21Z

Looks like we might need to add a type: ignore[union-attr] to the BooleanOptionalAction backport so the CI/CD passes.

Yup, done!

2. Argparse SubParser Backport Bug

Hmm this is an interesting one. Ideally I would like to keep the same behaviour (i.e., show the different command options across the different versions.

Default Static Metavar

This would be my favourite way to do this, but I understand that having every command as a metavar is more helpful than just "COMMAND".

Dynamic Metavar Using Property

Too messy, such that the code becomes less readable.

Backport and MonkeyPatch _get_action_name

I am usually not a fan of monkey patching module functions, but this is pretty clean, and we can turn it off on newer Python versions with sys.version_info check.

@kytta I think at the moment I prefer (3). What do you think?

Yeah, let's do that.

kytta · 2023-01-09T09:18:06Z

@SupImDos another thing I wanted to ask: How do you like typing_extensions and importlib_metadata to be imported? They currently use two different approaches, and I want to use the same one for both of them:

Version-checked. We would only install the backports on those Python versions, where they're needed, and check the version at runtime:

# foobar.py
if sys.version < (3, 8):
    import importlib_metadata as metadata
else:
    from importlib import metadata

# pyproject.toml

[tool.poetry.dependencies]
# [...]
importlib_metadata = { version = ">=4", python = "<3.8" }

Always import. We don't care about the Python version and always use the backport:

# foobar.py
import importlib_metadata as metadata

# pyproject.toml

[tool.poetry.dependencies]
importlib_metadata = ">=4"

I like the (1) better, as it allows us to ship less dependencies to the most users (as I don't think that many people use Python 3.7 in production, especially for command-line-apps). It is also very easy to remove later with pyupgrade. But the code is cleaner with the second option 🤔

SupImDos · 2023-01-09T09:40:39Z

How do you like typing_extensions and importlib_metadata to be imported?

Version-checked. We would only install the backports on those Python versions, where they're needed, and check the version at runtime...

This feels like the correct way to do it. As you say, it allows us to ship less dependencies to the most users.

Always import. We don't care about the Python version and always use the backport...

Less code branching is a lot cleaner and it is tempting. But I think (1) is still better in a practical sense.

@kytta I agree with you that we should go with (1) 😃

I am interested to see how the code coverage is calculated with branches that only execute in certain Python versions. We will have to see 🤔

kytta · 2023-01-09T10:32:44Z

I am interested to see how the code coverage is calculated with branches that only execute in certain Python versions. We will have to see 🤔

This is also an interesting moment.

A while ago I have noticed the comment # pragma: <3.8, cover somewhere, and it was supposed to disable coverage on the branches on specific Python repos. I have used it in my project, and it worked.

Now, I have tried to also use it here (see commit c2bcc6c), but it didn't work (at least locally). For me, poe test reports missing lines, these version checks to be exact.

I then tried to look this pragma up, but neither coverage nor pytest-cov docs mention anything other than pragma: no cover…

If we can't get this to work, I propose setting pragma: no cover on lines for Python 3.7–3.8 and only run coverage on the newer versions

SupImDos · 2023-01-09T11:26:00Z

@kytta
I've done a bit of a look into the # pragma: >x.y cover and I think I might have worked it out. As far as I can tell it looks like it comes from the covdefaults package.

I had a look at your venvy project which uses # pragma: <3.8 cover and # pragma: >=3.8 cover, and it looks like it has covdefaults installed.

I think this works? covdefaults looks pretty useful. Maybe we should do:

poetry add --group dev covdefaults

Modify pyproject.toml

[tool.coverage.run]
- branch = true
+ plugins = ["covdefaults"]
-
- [tool.coverage.report]
- show_missing = true
- exclude_lines = [
-   "if __name__ == .__main__.:",
- ]

Does this sound right? What do you think?

kytta · 2023-01-09T11:59:43Z

@kytta I've done a bit of a look into the # pragma: >x.y cover and I think I might have worked it out. As far as I can tell it looks like it comes from the covdefaults package.

I had a look at your venvy project which uses # pragma: <3.8 cover and # pragma: >=3.8 cover, and it looks like it has covdefaults installed.

I think this works? covdefaults looks pretty useful. Maybe we should do:
1. `poetry add --group dev covdefaults`

2. Modify `pyproject.toml`
   ```diff
   [tool.coverage.run]
   - branch = true
   + plugins = ["covdefaults"]
   -
   - [tool.coverage.report]
   - show_missing = true
   - exclude_lines = [
   -   "if __name__ == .__main__.:",
   - ]
   ```
Does this sound right? What do you think?

Oh right, I completely forgot about this package! Yeah, this looks good, I'll add this to pyproject.toml

kytta · 2023-01-17T14:38:00Z

Alright, everything is fixed, and the only thing left is the coverage for some of the backports:

Should we even cover those? I mean, it's not even our code... 🤔

SupImDos · 2023-01-20T05:19:19Z

@kytta
Thanks so much, this looks great!

Alright, everything is fixed, and the only thing left is the coverage for some of the backports
Should we even cover those? I mean, it's not even our code... 🤔

You're right, I would be happy with a # pragma: no cover for now, as you say its not our code anyway. I think the CI/CD is only failing because of the <100% coverage (which will be fixed by the pragmas) and a whitespace issue in patches.py.

I'll do my best to review this properly as soon as I can 😃

kytta · 2023-02-04T21:56:05Z

Alright, should be done now! All checks run fine on my machine 👌🏻

SupImDos

Looks great, thanks again for all this work!

kytta added 3 commits January 7, 2023 00:30

Make Python version smaller in pyproject.toml

1b3d860

Update lockfile

2a85981

Replace type imports with legacy versions

1550139

kytta mentioned this pull request Jan 7, 2023

Consider supporting older Python versions? #35

Closed

kytta added 7 commits January 7, 2023 12:57

Use get_args and get_origin from typing_extensions

0c43c36

Fix linting issues

b8da274

Don't use exit_on_error on older Python versions

88f4904

On error, the older versions of argparse call the error() method, which is already overridden in a way that we like. So, to fix the tests, we just do not provide the parameter to the constructor on older Python versions.

Backport BooleanOptionalAction

2cf59fb

Fix cast

7586065

Fix test_is_field_a

0303ba0

Fix importlib_metadata and typing_extensions more loosely

70ba0b5

kytta added 3 commits January 8, 2023 18:15

Fix typing_extensions not being imported everywhere

d36ce63

Fix coverage on sys.version_info

c2bcc6c

Fix coverage on argparse Parser init

9280060

kytta added 2 commits January 9, 2023 10:07

Fix unchanged type

602d161

Ignore error on backported BooleanOption

7f2d2f9

kytta added 5 commits January 17, 2023 14:55

Fix covdefaults

ddd56f1

Remove [toml] extra for pytest-cov

a4829d3

Use typing_extensions on Python 3.7 only

53c6ceb

Backport _get_action_name

41be193

Enable GitHub Actions on 3.7 and 3.8

f011a96

Update docs

38e1925

kytta marked this pull request as ready for review January 17, 2023 14:40

kytta added 2 commits February 4, 2023 22:54

Fix whitespace issue

c72bf45

Don't cover the backported CPython code

06f41be

SupImDos approved these changes Feb 6, 2023

View reviewed changes

SupImDos merged commit 1397310 into SupImDos:master Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make project compatible with Python 3.7+ #37

Make project compatible with Python 3.7+ #37

kytta commented Jan 7, 2023

kytta commented Jan 7, 2023

kytta commented Jan 7, 2023

SupImDos commented Jan 9, 2023

kytta commented Jan 9, 2023 •

edited

Loading

2. Argparse SubParser Backport Bug

kytta commented Jan 9, 2023 •

edited

Loading

SupImDos commented Jan 9, 2023

kytta commented Jan 9, 2023

SupImDos commented Jan 9, 2023

kytta commented Jan 9, 2023

kytta commented Jan 17, 2023

SupImDos commented Jan 20, 2023

kytta commented Feb 4, 2023

SupImDos left a comment

Make project compatible with Python 3.7+ #37

Make project compatible with Python 3.7+ #37

Conversation

kytta commented Jan 7, 2023

kytta commented Jan 7, 2023

kytta commented Jan 7, 2023

SupImDos commented Jan 9, 2023

1. Minor Type Checking Error

2. Argparse SubParser Backport Bug

kytta commented Jan 9, 2023 • edited Loading

2. Argparse SubParser Backport Bug

kytta commented Jan 9, 2023 • edited Loading

SupImDos commented Jan 9, 2023

kytta commented Jan 9, 2023

SupImDos commented Jan 9, 2023

kytta commented Jan 9, 2023

kytta commented Jan 17, 2023

SupImDos commented Jan 20, 2023

kytta commented Feb 4, 2023

SupImDos left a comment

Choose a reason for hiding this comment

kytta commented Jan 9, 2023 •

edited

Loading

kytta commented Jan 9, 2023 •

edited

Loading