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.

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

Conversation

YashRL
Copy link

@YashRL YashRL commented Mar 13, 2024

  1. Removed Future Import: Removed the from __future__ import annotations import, as it's unnecessary in Python 3.7 and later.

  2. Simplified Version Check: Simplified the version check for Python 3.10 or later.

  3. Removed Unused Imports: Removed unused imports to clean up the code.

  4. Simplified Type Annotation: Removed the type annotation for action.type in the format_action function to simplify the code.

  5. Improved Readability: Made minor formatting improvements to enhance readability.

Copy link
Owner

@joouha joouha left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to make this PR, I appreciate you trying to help make euporie better!

I've added some comments explaining why some things are the way they are, and why I don't want them changing.

If you want to revert the changes I've mentioned in my comments (changes to imports, removal of the cast call, and removal of new new-lines, I think we can probably get this merged for you.

Please ensure that the code passes formatting, linting and typing checks, and that the script actually runs before committing next time. You can do this by running:

$ hatch fmt
$ hatch run type:run
$ hatch run python scripts/document_cli_flags.py

and ensure all three commands run successfully.

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

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 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.


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.

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.

@@ -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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants