Skip to content

Conversation

@ndavidova
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Nov 30, 2025

Codecov Report

❌ Patch coverage is 69.05263% with 147 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.92%. Comparing base (cc87bc5) to head (efe91bc).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...c/sec_certs/heuristics/br1/table_parsing/parser.py 18.97% 47 Missing ⚠️
...sec_certs/heuristics/br1/chapter_parsing/mapper.py 20.00% 32 Missing ⚠️
...ec_certs/heuristics/br1/table_parsing/md_tables.py 22.59% 24 Missing ⚠️
...ts/heuristics/br1/chapter_parsing/chapter_utils.py 44.83% 16 Missing ⚠️
..._certs/heuristics/br1/chapter_parsing/validator.py 22.23% 14 Missing ⚠️
src/sec_certs/sample/fips.py 47.37% 10 Missing ⚠️
src/sec_certs/dataset/fips.py 55.56% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
+ Coverage   58.38%   58.92%   +0.55%     
==========================================
  Files          75       95      +20     
  Lines        8821     9296     +475     
==========================================
+ Hits         5149     5477     +328     
- Misses       3672     3819     +147     

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

@ndavidova ndavidova force-pushed the br1_extraction branch 3 times, most recently from b1edbea to 878c97e Compare December 1, 2025 09:52
Copy link
Member

@J08nY J08nY left a comment

Choose a reason for hiding this comment

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

Cool PR. Thanks for the work! Really nicely done. It adds a bunch of code, though I guess that is understandable and most of it is in the JSON describing the format and the table dataclasses. I am not sure whether sec_certs.br1 is the right package for this. I think it makes the most sense to have it under sec_certs.br1.heuristics? (wdyt @adamjanovsky ?) Even though it is not a heuristic necessarily (the matching is a bit of a heuristic). Just having it top-level feels weird given that it is a bit of a niche feature in the grand scheme of things.

Also, when #525 lands, there should be tests that show the successful parsing of a BR1 policy.



@dataclass
class ApprovedAlgo:
Copy link
Member

Choose a reason for hiding this comment

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

All of these DataClasses are quite verbose, though I like that it allows the type information to carry a lot of the structure and meta information about the data. Do you have any additional descriptions of these fields from the BR1? If so, could you (perhaps in an automated way or not) add them as docstrings to the classes and attributes? Just a single sentence (if available) would suffice.

The names are usually quite descriptive so this is not that important, more of a nice to have.

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.

3 participants