jazzband / prettytable

Display tabular data in a visually appealing ASCII table format
https://pypi.org/project/PrettyTable/
Other
1.35k stars 154 forks source link

Updated pre-commit with mypy #218

Closed phershbe closed 1 year ago

phershbe commented 1 year ago

Issue: https://github.com/jazzband/prettytable/issues/203

mypy is added to pre-commit but doesn't pass yet. The error messages are below. I'm working on them but anybody else is welcome to pitch in as well.

src/prettytable/prettytable.py:137: error: Need type annotation for "_field_names" (hint: "_field_names: List[<type>] = ...")
src/prettytable/prettytable.py:138: error: Need type annotation for "_rows" (hint: "_rows: List[<type>] = ...")
src/prettytable/prettytable.py:150: error: Need type annotation for "_widths" (hint: "_widths: List[<type>] = ...")
src/prettytable/prettytable.py:209: error: Need type annotation for "_none_format" (hint: "_none_format: Dict[<type>, <type>] = ...")
src/prettytable/prettytable.py:2042: error: Argument 2 to "dumps" has incompatible type "**Dict[str, object]"; expected "bool"
src/prettytable/prettytable.py:2042: error: Argument 2 to "dumps" has incompatible type "**Dict[str, object]"; expected "Optional[Type[JSONEncoder]]"
src/prettytable/prettytable.py:2042: error: Argument 2 to "dumps" has incompatible type "**Dict[str, object]"; expected "Union[None, int, str]"
src/prettytable/prettytable.py:2042: error: Argument 2 to "dumps" has incompatible type "**Dict[str, object]"; expected "Optional[Tuple[str, str]]"
src/prettytable/prettytable.py:2042: error: Argument 2 to "dumps" has incompatible type "**Dict[str, object]"; expected "Optional[Callable[[Any], Any]]"
src/prettytable/prettytable.py:2423: error: Need type annotation for "tables" (hint: "tables: List[<type>] = ...")
src/prettytable/prettytable.py:2424: error: Need type annotation for "last_row" (hint: "last_row: List[<type>] = ...")
src/prettytable/prettytable.py:2425: error: Need type annotation for "rows" (hint: "rows: List[<type>] = ...")
src/prettytable/__init__.py:52: error: Name "importlib_metadata" already defined (by an import)
src/prettytable/colortable.py:37: error: Self argument missing for a non-static method (or an invalid type for self)
tests/test_prettytable.py:304: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:305: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:309: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:310: error: prettytable? has no attribute "get_html_string"
tests/test_prettytable.py:314: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:315: error: prettytable? has no attribute "get_latex_string"
tests/test_prettytable.py:319: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:320: error: prettytable? has no attribute "field_names"
tests/test_prettytable.py:328: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:367: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:369: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:370: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:374: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:376: error: prettytable? has no attribute "get_html_string"
tests/test_prettytable.py:377: error: prettytable? has no attribute "get_html_string"
tests/test_prettytable.py:381: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:383: error: prettytable? has no attribute "get_latex_string"
tests/test_prettytable.py:384: error: prettytable? has no attribute "get_latex_string"
tests/test_prettytable.py:420: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:421: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:422: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:528: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:529: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:533: error: Module "prettytable" is not valid as a type
tests/test_prettytable.py:534: error: prettytable? has no attribute "get_string"
tests/test_prettytable.py:537: error: Incompatible types in assignment (expression has type "Set[int]", variable has type "List[int]")
tests/test_prettytable.py:784: error: "PrettyTable" has no attribute "caching"
Found 41 errors in 4 files (checked 5 source files)
codecov[bot] commented 1 year ago

Codecov Report

Merging #218 (4e3406b) into master (aa4e316) will increase coverage by 0.19%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   94.19%   94.38%   +0.19%     
==========================================
  Files           5        5              
  Lines        2258     2281      +23     
==========================================
+ Hits         2127     2153      +26     
+ Misses        131      128       -3     
Flag Coverage Δ
macos-latest 94.34% <92.85%> (+0.14%) :arrow_up:
ubuntu-latest 94.34% <92.85%> (+0.14%) :arrow_up:
windows-latest 94.30% <85.71%> (+0.23%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/prettytable/__init__.py 100.00% <100.00%> (ø)
src/prettytable/colortable.py 100.00% <100.00%> (ø)
src/prettytable/prettytable.py 90.69% <100.00%> (+0.26%) :arrow_up:
tests/test_prettytable.py 100.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

phershbe commented 1 year ago

@hugovk I started to make some type annotations and tried to upload them but Git wouldn't accept the commit with mypy not passing the pre-commit. I'm going to finish making them and try adding them tomorrow. I'm a little bit in over my head here but I'm starting to get it, I'll need it checked obviously but I'll also include questions on the things that I'm most uncertain about.

hugovk commented 1 year ago

Sure, let's tackle them bit by bit!


If you have pre-commit installed locally, and the commit fails because mypy isn't yet happy, you can bypass it using the -n flag:

-n, --no-verify       bypass pre-commit and commit-msg hooks

For example:

git commit -m "My commit message" -n

For this:

src/prettytable/__init__.py:52: error: Cannot find implementation or library
stub for module named "importlib_metadata"  [import]
        import importlib_metadata
    ^
src/prettytable/__init__.py:52: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
src/prettytable/__init__.py:52: error: Name "importlib_metadata" already
defined (by an import)  [no-redef]
        import importlib_metadata
        ^

That's coming from here:

try:
    # Python 3.8+
    import importlib.metadata as importlib_metadata
except ImportError:
    # <Python 3.7 and lower
    import importlib_metadata

It's getting a bit confused by the multiple imports. In real life, we can only ever import one of them, but I don't think mypy can handle this. So the usual thing to do is just tell mypy to ignore the second one:

 try:
     import importlib.metadata as importlib_metadata
 except ImportError:
     # <Python 3.7 and lower
-    import importlib_metadata
+    import importlib_metadata  # type: ignore
phershbe commented 1 year ago

@hugovk I'm working on it now. I said that I would submit it yesterday but I got delayed because I couldn't figure out why my git clone wasn't showing the changes in the pre-commit file. I was on the wrong branch, and finally got it figured out with git checkout. I'll have it in soon.

hugovk commented 1 year ago

All good! As before, absolutely no rush with this, let's take our time :)

This one:

src/prettytable/colortable.py:37: error: Self argument missing for a non-static
method (or an invalid type for self)  [misc]
        def format_code(s: str) -> str:

Looks like we've found a bug :) There is no self for that method and it doesn't modify anything from the class, so let's make it static:

diff --git a/src/prettytable/colortable.py b/src/prettytable/colortable.py
index dd5495a..625c89e 100644
--- a/src/prettytable/colortable.py
+++ b/src/prettytable/colortable.py
@@ -34,6 +34,7 @@ class Theme:
         self.junction_char = junction_char
         self.junction_color = Theme.format_code(junction_color)

+    @staticmethod
     def format_code(s: str) -> str:
         """Takes string and intelligently puts it into an ANSI escape sequence"""
         if s.strip() == "":
hugovk commented 1 year ago

For this:

src/prettytable/colortable.py:9: error: All conditional function variants must
have identical signatures  [misc]
        def init():
        ^

The code in question is:

try:
    from colorama import init
except ImportError:
    # Do nothing if not installed
    def init():
        pass

init()

Colorama is used to set up the terminal for printing colour, mainly needed for Windows where it can otherwise be a bit tricky.

But we don't require Colorama as a dependency, and so only call init if it happens to be installed as a dependency.

But let's not have to worry about keeping in sync the signatures of the real init with our fake one, and instead refactor it so remove our dummy init, something like:

diff --git a/src/prettytable/colortable.py b/src/prettytable/colortable.py
index dd5495a..e1224ea 100644
--- a/src/prettytable/colortable.py
+++ b/src/prettytable/colortable.py
@@ -4,13 +4,12 @@ from .prettytable import PrettyTable

 try:
     from colorama import init
+
+    init()
 except ImportError:
     # Do nothing if not installed
-    def init():
-        pass
-
+    pass

-init()

 RESET_CODE = "\x1b[0m"
hugovk commented 1 year ago

This one is like setuptools and Colorama, no type hints in the module:

src/prettytable/prettytable.py:48: error: Cannot find implementation or library
stub for module named "wcwidth"  [import]
    import wcwidth

Except unfortunately there are no type hints on Typeshed either. I think we can get away with just ignoring this one:

diff --git a/src/prettytable/prettytable.py b/src/prettytable/prettytable.py
index 18379ce..29fe714 100644
--- a/src/prettytable/prettytable.py
+++ b/src/prettytable/prettytable.py
@@ -45,7 +45,7 @@ from html import escape
 from html.parser import HTMLParser
 from typing import Any

-import wcwidth
+import wcwidth  # type: ignore

 # hrule styles
 FRAME = 0
phershbe commented 1 year ago

@hugovk Okay, I finally finished. I got the static method correction by myself but was super stuck on a few others and then realized that you had made notes about them above, awesome. I'm not super confident about types that I added, they make mypy pass so I guess that's a good sign, but you might want to check them.

phershbe commented 1 year ago

@hugovk It seems that I had forgotten to change this from a draft pull request to a pull request, sorry about my confusion. It should be ready now!

hugovk commented 1 year ago

Thank you, merged! 🚀