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

Add type hints from typeshed #205

Closed phershbe closed 1 year ago

phershbe commented 1 year ago

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

I didn't do the third part yet... Add mypy via pre-commit and make mypy pass (can be follow-up PR if there's much to do) ...as I'm newer to contributing and didn't want to get overwhelmed. I can open a new issue so that it doesn't get forgotten and work on that next.

Used merge_pyi to integrate the type annotations and added appropriate args to .pre-commit-config.yaml in order for pre-commit to add the import to Python files so that newer typing syntax can be used on older Python 3.7 (as described in the issue).

Note: Did not use Black to format as it was deleting spaces in the docstrings which presumably are supposed to be there; can use Black and then add the spaces again after if that's better.

hugovk commented 1 year ago

Now we have from __future__ import annotations on the files, we're using the newer syntax even for older Python versions like 3.7.

That means in tests/test_prettytable.py, we no longer need the existing from typing import List and can refer to plain list instead of List further down.

So please also make these changes:

-from typing import Any, List
+from typing import Any
     def test_break_line_ASCII(
-        self, rows: List[List[Any]], hrule: int, expected_result: str
+        self, rows: list[list[Any]], hrule: int, expected_result: str
     ):
phershbe commented 1 year ago

@hugovk Awesome, thank you, this feedback is invaluable to me. I was sick on the weekend so I'm working on this now, I'll have it done today.

phershbe commented 1 year ago

Now we have from __future__ import annotations on the files, we're using the newer syntax even for older Python versions like 3.7.

That means in tests/test_prettytable.py, we no longer need the existing from typing import List and can refer to plain list instead of List further down.

So please also make these changes:

-from typing import Any, List
+from typing import Any
     def test_break_line_ASCII(
-        self, rows: List[List[Any]], hrule: int, expected_result: str
+        self, rows: list[list[Any]], hrule: int, expected_result: str
     ):

Done. If I'm not mistaken, the change in test_break_line_ASCII must have already been done automatically when I used merge_pyi.

phershbe commented 1 year ago

@hugovk I made the changes and changed the request from draft PR to PR. I'm a little bit nervous to break something since I don't have much experience, does the pre-commit check that more or less?

I also created an issue at https://github.com/jazzband/prettytable/issues/206 for the unfinished part of the issue that this pull request is based upon so that there would be no way for it to get ignored, even though I'm starting to work on it now.

codecov[bot] commented 1 year ago

Codecov Report

Merging #205 (11f0435) into master (8dba696) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   94.03%   94.05%   +0.01%     
==========================================
  Files           5        5              
  Lines        2248     2254       +6     
==========================================
+ Hits         2114     2120       +6     
  Misses        134      134              
Flag Coverage Δ
macos-latest 94.05% <100.00%> (+0.01%) :arrow_up:
ubuntu-latest 94.05% <100.00%> (+0.01%) :arrow_up:
windows-latest 93.92% <100.00%> (+0.01%) :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.20% <100.00%> (+0.01%) :arrow_up:
tests/test_colortable.py 100.00% <100.00%> (ø)
tests/test_prettytable.py 100.00% <100.00%> (ø)

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

hugovk commented 1 year ago

Thank you!

Yes, pre-commit runs tools like linters and formatters to check the code is formatted well, and for some types of errors (this is defined in .pre-commit-config.yaml).

Plus a test suite is run, which runs the unit tests with six versions of Python on three operating systems (they're in the tests directory).

And I'll review the changes to make sure they look good. 👍

phershbe commented 1 year ago

@hugovk wow I can't thank you enough for how professional you've been answering all of my questions. Getting started making contributions to actual projects is intimidating, and having help like this is awesome. I will pay it forward and help other people when I get much better.

hugovk commented 1 year ago

You're very welcome, I'm happy to help out :)