banesullivan / scooby

🐶 🕵️ Great Dane turned Python environment detective
MIT License
47 stars 12 forks source link

add type hints #109

Closed Gryfenfer97 closed 11 months ago

Gryfenfer97 commented 1 year ago

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

banesullivan commented 1 year ago

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

Gryfenfer97 commented 1 year ago

@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 commented 12 months ago

Codecov Report

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

:exclamation: 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 commented 12 months ago

@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 commented 12 months ago

@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 commented 12 months ago

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

banesullivan commented 12 months ago

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 commented 12 months ago

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

Gryfenfer97 commented 11 months ago

I hope this commit fix all the issues

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

banesullivan commented 11 months ago

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 commented 11 months ago

Looks like 3.8 is still failing on CI for some reason

I hope this one fix everything