Another typing attempt #199
Conversation
FlavioAmurrioCS
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #199 +/- ##
==========================================
+ Coverage 98.12% 98.24% +0.11%
==========================================
Files 12 13 +1
Lines 2350 2452 +102
==========================================
+ Hits 2306 2409 +103
+ Misses 44 43 -1 ☔ View full report in Codecov by Sentry. |
|
Thanks for the persistence! I took a quick lookover PR and initially it looks good. I'll take a second look once the tests are passing. It would be great to get the code cov tests passing, but if there is a good reason they wouldn't pass due to lower coverage, then thats fine, wouldn't be a blocker. |
…st for remove none
|
@EntilZha I went ahead configured pytest-cov to ignore TYPE_CHECKING blocks and ellipses. nedbat/coveragepy#831 (comment) so hope things should be good now. |
| encoding: Optional[str] = None, | ||
| errors: Optional[str] = None, | ||
| newline: Optional[str] = None, | ||
| encoding: str | None = None, |
There was a problem hiding this comment.
Python supports the pipe symbol now "I"
So, Optional[str] and str | None are pretty much the same as far as I know
There was a problem hiding this comment.
So there isn't any point in changing these parts, is there? Unless I am missing something
There was a problem hiding this comment.
They are identical for typing purposes as far as I know people gravitate to the pipe version due to it being shorter and more concise. Reason I change existing ones was because I ran ruff on this code with the following rule. https://docs.astral.sh/ruff/rules/non-pep604-annotation/
If allowed I am planning to introduce ruff to this project in the future since I have found it very helpful in my personal projects.
There was a problem hiding this comment.
In general, I prefer the str | None, but one argument in favor of the other style is that the pipe is only available in python 3.10 and above. I'm personally fine with dropping support for python 3.8/3.9 in favor of better syntax, but would be accepting of the old syntax if its a substantial barrier to current use.
There was a problem hiding this comment.
No need to drop support for 3.8/3.9. With from __future__ import annotations it allows us to use the new syntax. https://peps.python.org/pep-0563/
| @@ -234,7 +270,7 @@ def cache(self, delete_lineage=False): | |||
| self._lineage = Lineage(engine=self.engine) | |||
| return self | |||
|
|
|||
| def head(self, no_wrap: Optional[bool] = None): | |||
| def head(self, no_wrap: bool | None = None) -> _T_co: | |||
There was a problem hiding this comment.
This is what I was referring to in the comment I left above
|
Hello @FlavioAmurrioCS |
|
This PR is ready for review. I am just waiting for feedback or an approval at this point. I do have more changes for the remaining files but didn't want to add too much to this PR to prevent it from being too large. cc: @EntilZha @maestro-1 |
|
Sorry for the delay, was traveling! LGTM to merge and passes all the test now. Thanks so much for pushing this across the finish line! |
Pull Request: Enhance Typing Support for Library
I've been a fan of this library for quite some time, but I've eagerly awaited proper typing support. After reviewing other pull requests, I noticed that some were abandoned or contained extensive changes. With that in mind, I've kept this pull request concise and focused.
Key Updates:
pre-commit-config.yamlwith Black formatting, mypy type checking, and ruff. (If possible please enable https://pre-commit.ci/ for this repo)pyproject.tomlfor mypy and ruff, enabling only a select few rules to avoid excessive modifications.remove_none: this will remote None values from stream as well as TypeGuard the stream.I hope this contribution aligns with the project's goals and makes it easier for everyone to work with! 🚀