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

refactor: add type hints in version_scanner.py #1581

Merged
merged 5 commits into from Mar 10, 2022

Conversation

rhythmrx9
Copy link
Contributor

part of #1539

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.

Untyped:

results = [dict()]

I don't understand why it has different values in two if branches...

cve_bin_tool/version_scanner.py Outdated Show resolved Hide resolved
cve_bin_tool/version_scanner.py Outdated Show resolved Hide resolved
score=0,
should_extract: bool = False,
exclude_folders: list[str] = [],
checkers: dict[str, type[Checker]] | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it type[Checker] and not just Checker? 🤔

Copy link
Contributor Author

@rhythmrx9 rhythmrx9 Feb 19, 2022

Choose a reason for hiding this comment

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

Because its the Checker subclass itself (uninstantiated) of type CheckerMetaClass.

for (dummy_checker_name, checker) in self.checkers.items():
checker = checker()

cve_bin_tool/version_scanner.py Outdated Show resolved Hide resolved
@rhythmrx9
Copy link
Contributor Author

rhythmrx9 commented Feb 19, 2022

Untyped:

results = [dict()]

I don't understand why it has different values in two if branches...

I am confused there as well, I don't think we need the for loop under as well.

Do we need this part?

if "is_or_contains" in result:
results = [dict()]
results[0] = result
else:
results = result
for result in results:

Tried to remove it, long tests for kerberos checker failed.
Some checkers override get_version method and return list of dictionaries(e.g. kerberos), we do need above part.
That is why results has different values.

@@ -33,15 +36,16 @@ class VersionScanner:
""" "Scans files for CVEs using CVE checkers"""

CHECKER_ENTRYPOINT = "cve_bin_tool.checker"
Infogen = Iterator[Tuple[Union[ProductInfo, None], str]]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very self explanatory. It needs documentation, or perhaps we need some appropriately named custom types to make this clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make it a custom type like ProductInfo, hope that makes it clearer.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

I think we might want to do some additional refinement with custom types, but this looks good enough to merge now. Thank you!

@terriko terriko merged commit 69f489d into intel:main Mar 10, 2022
@rhythmrx9 rhythmrx9 deleted the type_hints_version branch March 12, 2022 15:34
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.

None yet

3 participants