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.

Sign up for GitHub

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

Jump to bottom

refactor: add type hints in util.py #1572

Merged
merged 12 commits into from Feb 22, 2022
Merged

refactor: add type hints in util.py #1572

merged 12 commits into from Feb 22, 2022

Conversation

rhythmrx9
Copy link
Contributor

@rhythmrx9 rhythmrx9 commented Feb 7, 2022 •
edited

part of #1539

Most of the file already had type hints.
@terriko I did not use ClassVar for class variables as it wasn't already used, Should I add it?

@rhythmrx9 rhythmrx9 changed the title refactor: added type hints in util.py refactor: add type hints in util.py Feb 7, 2022
@rhythmrx9 rhythmrx9 marked this pull request as ready for review February 10, 2022 05:37
@rhythmrx9
Copy link
Contributor Author

Changed the latest commit message to follow Conventional Commits.

Molkree
Molkree suggested changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@Molkree Molkree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely should add type checking to CI and pre-commit, it'll make reviewing and adding type hints easier.

cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
cve_bin_tool/util.py Outdated Show resolved Hide resolved
@Molkree
Copy link
Contributor

Molkree commented Feb 10, 2022

I did not use ClassVar for class variables as it wasn't already used, Should I add it?

@rhythmrx9, oh, where did you want to use it? I'm not super familiar with the concept.

rhythmrx9 and others added 7 commits February 11, 2022 11:31
@rhythmrx9 @Molkree
refactor: arguments to new __new__
521724e 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: CVEData
f5dac76 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: simplifying type hints
51cb63e 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: type hints in regex_find
24b960a 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: imports in util.py
316cd8b 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: simplifying type hints
583202b 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9 @Molkree
refactor: simplifying type hints
fefb212 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9
Copy link
Contributor Author

rhythmrx9 commented Feb 11, 2022 •
edited

oh, where did you want to use it? I'm not super familiar with the concept.

I wanted to use it for class variables like this,

class Sample: 
        x: ClassVar[int]

But now thinking back, and looking at your suggestions, I think it's not needed.

Molkree
Molkree suggested changes Feb 11, 2022
View reviewed changes
cve_bin_tool/util.py Outdated Show resolved Hide resolved
rhythmrx9 and others added 2 commits February 12, 2022 09:13
@rhythmrx9 @Molkree
fix: inline type hints
6d60d01 
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
@rhythmrx9
fix: imports for inline type hints
ff1a8a7
terriko
terriko reviewed Feb 16, 2022
View reviewed changes
cve_bin_tool/util.py
class CVEData(defaultdict):
def __missing__(self, key):
class CVEData(DefaultDict[str, Union[List[CVE], Set[str]]]):
def __missing__(self, key: str) -> list[CVE] | set[str]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this one have a return type? I'm not sure if those are needed for __ functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do already use type hints for __ functions, I think we should have them, to be consistent.

@terriko
Copy link
Contributor

terriko commented Feb 16, 2022

This is looking good. I've approved CI to run. I don't think we need to use ClassVar at this time. And I agree that we should probably put some type checking CI stuff in place to make reviewing these easier. If anyone's got a preferred solution for that, let me know.

@codecov-commenter
Copy link

Codecov Report

Merging #1572 (ff1a8a7) into main (62ae407) will decrease coverage by 0.05%.
The diff coverage is 72.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1572      +/-   ##
==========================================
- Coverage   80.36%   80.30%   -0.06%     
==========================================
  Files         281      281              
  Lines        5581     5590       +9     
  Branches      913      916       +3     
==========================================
+ Hits         4485     4489       +4     
- Misses        897      900       +3     
- Partials      199      201       +2
Flag Coverage Δ
longtests 80.30% <72.41%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/cli.py 70.79% <33.33%> (-0.64%) ⬇️
cve_bin_tool/version_scanner.py 56.86% <50.00%> (+0.42%) ⬆️
test/test_checkers.py 93.54% <66.66%> (-3.07%) ⬇️
cve_bin_tool/util.py 75.00% <87.50%> (-0.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d95852...ff1a8a7. Read the comment docs.

@terriko terriko merged commit 2b04fe6 into intel:main Feb 22, 2022
@rhythmrx9 rhythmrx9 deleted the type_hints_util branch March 7, 2022 14:36
terriko pushed a commit to terriko/cve-bin-tool that referenced this pull request Mar 9, 2022
@rhythmrx9 @Molkree @terriko
refactor: add type hints in util.py (intel#1572)
4762723 
* related to intel#1539

* refactor: type hints in util.py

* refactor: arguments to new __new__

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: CVEData

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: simplifying type hints

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: type hints in regex_find

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: imports in util.py

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: simplifying type hints

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: simplifying type hints

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* refactor: improving type hints in util.py

* fix: inline type hints

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

* fix: imports for inline type hints

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Molkree Molkree Molkree requested changes

@terriko terriko terriko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rhythmrx9 @Molkree @terriko @codecov-commenter