Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize document_cli_flags.py working on issue #81 #103

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Optimize document_cli_flags.py working on issue #81 #103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions scripts/document_cli_flags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,25 @@
#!/usr/bin/env python
"""Fill in the CLI reference in euporie's documentation."""

from __future__ import annotations

import subprocess
import sys
from textwrap import dedent, indent
from typing import TYPE_CHECKING, cast
from typing import TYPE_CHECKING, Callable
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've removed the cast call where Callable is used, so this import would no longer be necessary.


if TYPE_CHECKING:
import argparse
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove from __future__ import annotations, then the argparse import would need to move outside of the type-checking import block, as without future imports, type annotations get evaluated at run-time.

With your changes, this script actually fails to run with a NameError:

Traceback (most recent call last):
  File "/home/josiah/data/projects/euporie/scripts/document_cli_flags.py", line 17, in <module>
    def format_action(action: argparse.Action) -> str:
                              ^^^^^^^^
NameError: name 'argparse' is not define

I've made the decision to use future imports throughout this codebase, and do not want to stop using it for this one script. I decided to use it because I use modern type annotation syntax for type checking (e.g. dict[list] instead of Dict[List], float | int | None instead of Optional[Union[float, int]], etc.), but I still want to support Python 3.8. By using future annotations, this means annotations are not evaluated at runtime and thus do not cause an error on older Python versions where they are not supported.

from typing import Callable

if sys.version_info[0] >= 3 and sys.version_info[1] >= 10:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to this change, I guess it is a little more readable.

if sys.version_info[:2] >= (3, 10):
from importlib.metadata import entry_points
else:
from importlib_metadata import entry_points


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using ruff to automatically format all Python scripts in this project. Ruff follows black's formatting style, which requires two new-lines between top level code blocks. Also I don't think it helps improve readability, so I don't want to remove of the double newlines.

def format_action(action: argparse.Action) -> str:
"""Format an action as RST."""
s = ""
type_ = ""
if action.type and action.type != bool:
action.type = cast("Callable", action.type)
type_ = f"<{action.type.__name__}>" # typing: ignore
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use mypy to type check my code-base for euporie. Removing the cast call results in the following mypy error:

scripts/document_cli_flags.py:22: error: Item "FileType" of "Callable[[str], Any] | FileType" has no attribute "__name__"  [union-attr]

The reason for this is that the argparse action type attribute can be a callable (a function) or a FileType, and object of the FileType type do not have a __name__ attribute.

However, here I know that the action type is a callable, as I have defined them, hence the need for the cast. I guess we could use a if callable(action.type): condition instead to make mypy happy, but I particularly object to using cast in this script.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The # typing: ignore comment is no longer necessary with a recent mypy version, so I'm happy for that to be removed.

type_ = f"<{action.type.__name__}>"
if action.choices:
type_ = f"{{{','.join(map(str, action.choices))}}}"

Expand All @@ -38,18 +33,12 @@ def format_action(action: argparse.Action) -> str:
"""
return s


def format_parser(
title: str, parser: argparse.ArgumentParser, description: str = ""
) -> str:
"""Format a parser's arguments as RST."""
s = ""

# s = "\n"
# s += ("*" * len(title)) + "\n" + title + "\n" + ("*" * len(title)) + "\n\n"
# s += description or dedent(parser.description or "").strip()
# s += "\n\n"

s += "\nUsage\n=====\n\n"
s += ".. code-block:: console\n\n"
usage = parser.format_usage()
Expand Down Expand Up @@ -77,7 +66,6 @@ def format_parser(

return s


if __name__ == "__main__":
for script in entry_points(group="console_scripts"):
if script.module.split(".")[0] == "euporie":
Expand All @@ -100,4 +88,4 @@ def format_parser(
break
break
else:
subprocess.call([sys.executable, __file__, script.name]) # S603
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually configured ruff to ignore S603, so this is fine to remove.

subprocess.call([sys.executable, __file__, script.name])