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

fix: mypy-errors in helper_script.py (#2763) #2873

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Conversation

Tengas
Copy link
Contributor

@Tengas Tengas commented Mar 29, 2023

Resolved all but one type-error in helper_script.py

The remaining mypy-error is due to line 264:
cursor = CVEDB.db_open_and_get_cursor(self)
which gives error cve_bin_tool/helper_script.py:262: error: Argument 1 to "db_open_and_get_cursor" of "CVEDB" has incompatible type "HelperScript"; expected "CVEDB" [arg-type]. Later, we also close this connection with CVEDB.db_close(self)

I can think of three ways to resolving this type error:

  1. Refactor, either made the affected functions CVEDB.db_open_and_get_cursor() and CVEDB.db_close() static in CVEDB.
  2. Inhertiance, we can make Helper_Script a subclass of CVEDB so it will inherit the relevant functions.
  3. Repeat the code, we rewrite the relevant parts of db_open_and_get_cursor() and db_close() in the helper_script.py.

@terriko
Copy link
Contributor

terriko commented Aug 24, 2023

Hey, sorry about completely dropping the ball on this pull request. I'm going to update the branch and see if this is still viable to merge.

@terriko terriko added awaiting maintainer Need a maintainer to respond / help out awaiting CI labels Aug 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023

Codecov Report

Merging #2873 (8336e52) into main (6d0c925) will decrease coverage by 0.48%.
Report is 1 commits behind head on main.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #2873      +/-   ##
==========================================
- Coverage   81.33%   80.86%   -0.48%     
==========================================
  Files         724      724              
  Lines       11347    11348       +1     
  Branches     1544     1544              
==========================================
- Hits         9229     9176      -53     
- Misses       1699     1752      +53     
- Partials      419      420       +1     
Flag Coverage Δ
longtests 75.59% <85.71%> (-0.01%) ⬇️
win-longtests 78.81% <57.14%> (-0.49%) ⬇️

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

Files Changed Coverage Δ
cve_bin_tool/helper_script.py 82.55% <85.71%> (+0.50%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Merge time. Thank you again for your work on this and your extreme patience while it got backburnered for a while!

@terriko terriko merged commit c855deb into intel:main Sep 21, 2023
21 checks passed
@Tengas
Copy link
Contributor Author

Tengas commented Jan 8, 2024

No worries, glad it was useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting CI awaiting maintainer Need a maintainer to respond / help out
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants