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

Add type hints from typeshed #205

Merged
merged 9 commits into from Oct 18, 2022
Merged

Add type hints from typeshed #205

merged 9 commits into from Oct 18, 2022

Conversation

phershbe
Copy link
Contributor

Issue: #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 hugovk added the changelog: Added For new features label Oct 14, 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.

Thank you!

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.

Yep, let's keep the scope narrower for this first PR. 👍

Here's some review comments.

src/prettytable/prettytable.py Outdated Show resolved Hide resolved
src/prettytable/colortable.py Outdated Show resolved Hide resolved
src/prettytable/prettytable.py Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Oct 14, 2022

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

@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 and others added 5 commits October 17, 2022 21:50
Remove _typeshed reference as we don't have access to it and we can use typing.Any instead.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Removed Callable, ClassVar, and Incomplete since they aren't used in the file.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Changed Incomplete to Any since we don't have access to the _typeshed module.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Removed import of "List" from "typing" since we now have access to "list" from the "from __future__ import annotations" line
@phershbe
Copy link
Contributor Author

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 phershbe marked this pull request as ready for review October 18, 2022 02:30
@phershbe
Copy link
Contributor Author

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

codecov bot commented Oct 18, 2022

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%) ⬆️
ubuntu-latest 94.05% <100.00%> (+0.01%) ⬆️
windows-latest 93.92% <100.00%> (+0.01%) ⬆️

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

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

@hugovk
Copy link
Member

hugovk commented Oct 18, 2022

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

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.

Thank you for your contribution!

@hugovk hugovk added the hacktoberfest-accepted To credit accepted Hacktoberfest PRs label Oct 18, 2022
@hugovk hugovk merged commit 409be53 into jazzband:master Oct 18, 2022
@phershbe
Copy link
Contributor Author

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

hugovk commented Oct 18, 2022

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

@hugovk hugovk changed the title Added type hints from typeshed Add type hints from typeshed Oct 28, 2022
@hugovk hugovk mentioned this pull request Nov 28, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features hacktoberfest-accepted To credit accepted Hacktoberfest PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants