Skip to content

Conversation

@mastersans
Copy link
Member

@mastersans mastersans commented Feb 11, 2024

fixes: #2821
problem fixed :

  • format of Toml including boolean values and list of values as string rather than lower case boolean values and normal list.
  • improvement in variable naming, usage of spaces for indentation in yaml, apparently yaml don't like tabs for indentation ; (

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2024

Codecov Report

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

Comparison is base (d6cbe40) 75.41% compared to head (3a2c871) 80.95%.
Report is 19 commits behind head on main.

Files Patch % Lines
cve_bin_tool/sbom_detection.py 76.00% 4 Missing and 2 partials ⚠️
cve_bin_tool/config_generator.py 90.90% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3803      +/-   ##
==========================================
+ Coverage   75.41%   80.95%   +5.53%     
==========================================
  Files         808      809       +1     
  Lines       11983    12080      +97     
  Branches     1598     1630      +32     
==========================================
+ Hits         9037     9779     +742     
+ Misses       2593     1879     -714     
- Partials      353      422      +69     
Flag Coverage Δ
longtests 80.41% <91.00%> (+4.99%) ⬆️
win-longtests 78.74% <91.00%> (?)

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
Collaborator

@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.

This is looking good, but it looks like you've removed all the comment text at the top of the auto-generated file. I don't mind making that a bit shorter, but we should probably at least keep the link telling people where to read more about the options.

Copy link
Collaborator

@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.

This is looking really good -- a very clear improvement over the original since it'll be able to handle new config options.

There might be some room for improvement in terms of human readability still: for example, I think most similar config generators put the comment with help above the config option rather than below, and it's usually common to put a space immediately after the #. But I think that I'd rather merge this and save formatting tweaks for a separate PR, so I'll file another issue for that rather than delaying this any longer.

Thank you!

@terriko terriko merged commit 171c480 into intel:main Feb 15, 2024
@mastersans mastersans deleted the i2821 branch February 15, 2024 18:32
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.

Making the config generator more robust to config option changes

3 participants