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 #109

Merged
merged 9 commits into from
Oct 1, 2023
Merged

add type hints #109

merged 9 commits into from
Oct 1, 2023

Conversation

Gryfenfer97
Copy link
Contributor

@Gryfenfer97 Gryfenfer97 commented Jun 27, 2023

add type hints

I am still not sure about the type of extra_meta in Report because there is check about it being a list.

Should fix #108

scooby/knowledge.py Outdated Show resolved Hide resolved
@banesullivan
Copy link
Owner

Thank you for contributing this, @Gryfenfer97! I'll try to review merge this soon

@Gryfenfer97
Copy link
Contributor Author

Gryfenfer97 commented Jul 17, 2023

@banesullivan I still need to import Literal from typing_extension for python 3.7.
However this version is no longer supported so if you still want to support it, tell me so I can add the correct import

I also recommend you to use a static type checker when looking at the code as it reveal some problems:
ex: L363 of report.py where self._extra_meta is defined as Optional and so, can be None

for meta in self._extra_meta:
    html, i = cols(html, meta[1], meta[0], self.ncol, i)

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2023

Codecov Report

Merging #109 (d0f0666) into main (9a0dec1) will increase coverage by 0.58%.
The diff coverage is 85.96%.

❗ 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     

@banesullivan
Copy link
Owner

@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

@Gryfenfer97
Copy link
Contributor Author

@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.
My question was for python 3.7, do you still want to support it or not ? If yes I have to add typing_extension as a dependency.

@banesullivan
Copy link
Owner

Ah, let me see if we can drop support for 3.7

@banesullivan
Copy link
Owner

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?

@Gryfenfer97
Copy link
Contributor Author

Gryfenfer97 commented Sep 24, 2023

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

@banesullivan banesullivan mentioned this pull request Sep 29, 2023
@Gryfenfer97
Copy link
Contributor Author

I hope this commit fix all the issues

By the way, there is a bot that can apply pre-commit automatically.

@banesullivan
Copy link
Owner

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

@Gryfenfer97
Copy link
Contributor Author

Looks like 3.8 is still failing on CI for some reason

I hope this one fix everything

Copy link
Owner

@banesullivan banesullivan left a 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!

@banesullivan banesullivan merged commit 91614ce into banesullivan:main Oct 1, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report additional and core argument type
3 participants