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

feat: Adding language parser for RPM file #3845

Closed
wants to merge 6 commits into from

Conversation

tahifahimi
Copy link
Contributor

Close #2916

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (d6cbe40) 75.41% compared to head (4e5ca3a) 80.70%.
Report is 29 commits behind head on main.

Files Patch % Lines
cve_bin_tool/parsers/rpm.py 71.83% 26 Missing and 14 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3845      +/-   ##
==========================================
+ Coverage   75.41%   80.70%   +5.29%     
==========================================
  Files         808      810       +2     
  Lines       11983    12239     +256     
  Branches     1598     1657      +59     
==========================================
+ Hits         9037     9878     +841     
+ Misses       2593     1915     -678     
- Partials      353      446      +93     
Flag Coverage Δ
longtests 75.51% <69.93%> (+0.09%) ⬆️
win-longtests 78.93% <70.42%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thanks for looking into this! I need to go look at the original PR and see if we can merge it and then yours so that the original contribution team gets appropriate credit in git, and it's probably not going to happen for a few days. But in the meantime, here's some cleanup requests:

  • please remove the commented out code from the PR. I don't mind having a bit of commented code around if it useful for debugging but I think you're probably done with the parts you have commented out here.
  • Is it possible to replace test_scan_files_multiline with something that tests the multiline capability and actually works? If you're not up to doing it, can you file an issue saying it needs a replacement? It needs one even now since it's failing on windows.
  • If nothing uses those condensed-download/*.rpm files, they should be removed in this PR alongside the test.

@tahifahimi
Copy link
Contributor Author

Nw, I like working on this project.
I totally understand, and it is a better idea to merge their PR since I used their code as the base of my RPM parser. So I think it would be a good idea to close this PR. I consider that the rpm.py is added to cve_bin_tool/parser/ folder based on #2964, and I will open a new PR. In the new PR, I will apply the requested notes. I can work on a test that we replace it with test_scan_files_multiline, so we can keep the condensed-download/*.rpm files, if we can have a set of tests for testing multiline capability.

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.

feat: "language" parser for rpm SPEC files
3 participants