-
Notifications
You must be signed in to change notification settings - Fork 12
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 #109
add type hints #109
Conversation
Thank you for contributing this, @Gryfenfer97! I'll try to review merge this soon |
@banesullivan I still need to import Literal from I also recommend you to use a static type checker when looking at the code as it reveal some problems: for meta in self._extra_meta:
html, i = cols(html, meta[1], meta[0], self.ncol, i) |
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #109 +/- ##
==========================================
+ Coverage 85.03% 85.61% +0.58%
==========================================
Files 5 5
Lines 441 445 +4
==========================================
+ Hits 375 381 +6
+ Misses 66 64 -2 |
@Gryfenfer97, apologies for my delay in addressing this. I'm curious if you'd still like to land these changes? There appears to be a few CI testing issues |
The issue in Python 3.8 was not intentional. Fixed now. |
Ah, let me see if we can drop support for 3.7 |
Given 3.7 is past end of life and PyVista (the biggest downstream dependency) has also dropped 3.7, we are good to remove it. Would you like me to handle that in a separate PR or would you be up for handling it here? |
I think it would make more sense in the git history to have a separate commit for it. So doing it in a separate PR would be better in my opinion |
I hope this commit fix all the issues By the way, there is a bot that can apply pre-commit automatically. |
I'm aware of the pre-commit bot, but this repo is generally pretty low maintenance so I haven't wanted the overhead of adding it. Looks like 3.8 is still failing on CI for some reason |
I hope this one fix everything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these additions and your patience!
add type hints
I am still not sure about the type of
extra_meta
inReport
because there is check about it being a list.Should fix #108