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

Updated pre-commit with mypy #218

Merged
merged 5 commits into from Dec 7, 2022
Merged

Updated pre-commit with mypy #218

merged 5 commits into from Dec 7, 2022

Conversation

phershbe
Copy link
Contributor

Issue: #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
Copy link

codecov bot commented Nov 10, 2022

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%) ⬆️
ubuntu-latest 94.34% <92.85%> (+0.14%) ⬆️
windows-latest 94.30% <85.71%> (+0.23%) ⬆️

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%) ⬆️
tests/test_prettytable.py 100.00% <0.00%> (ø)

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

@phershbe
Copy link
Contributor Author

@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
Copy link
Member

hugovk commented Nov 10, 2022

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 and others added 2 commits November 10, 2022 21:03
added "pretty" and "show-error-codes" arguments and excluded "tests"

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Added Colorama and Setuptools as dependencies to mypy in pre-commit.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@phershbe
Copy link
Contributor Author

@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
Copy link
Member

hugovk commented Nov 11, 2022

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
Copy link
Member

hugovk commented Nov 11, 2022

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
Copy link
Member

hugovk commented Nov 11, 2022

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
Copy link
Contributor Author

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

@hugovk hugovk added the changelog: Added For new features label Nov 28, 2022
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Good stuff! Just a few little comments and this is good for merge.

src/prettytable/prettytable.py Show resolved Hide resolved
src/prettytable/prettytable.py Show resolved Hide resolved
src/prettytable/prettytable.py Show resolved Hide resolved
src/prettytable/prettytable.py Show resolved Hide resolved
@hugovk hugovk mentioned this pull request Nov 28, 2022
4 tasks
@phershbe phershbe marked this pull request as ready for review December 6, 2022 19:14
@phershbe
Copy link
Contributor Author

phershbe commented Dec 6, 2022

@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 hugovk merged commit 4152432 into jazzband:master Dec 7, 2022
@hugovk
Copy link
Member

hugovk commented Dec 7, 2022

Thank you, merged! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants