chore(SPDX SBOM): Refactor to_cyclonedx()#1613
Conversation
Over the past year to_cyclonedx() has grown significantly to accommodate various corner-cases. This change splits the function into several logically isolated chunks to keep it compact and easy to understand. Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the to_cyclonedx method in hermeto/core/models/sbom.py by extracting several helper functions to improve readability and code structure. The review feedback suggests simplifying convert_annotations to comply with PEP 8 and avoid type-ignore comments, updating the type hint of purls in is_a_wrapper to prevent potential truthiness bugs with generic iterables, and aligning parameter types in partial_component with the Component constructor to satisfy static type checkers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def is_a_wrapper(purls: Iterable[str], package_name: str, package_version_info: str | None) -> bool: | ||
| """Check wrapper package properties.""" | ||
|
|
||
| # if there's no purl and no package name or version, it's just wrapping element for | ||
| # spdx package which is one layer below SPDXDocument in relationships | ||
| return not any((purls, package_name, package_version_info)) |
There was a problem hiding this comment.
The purls parameter is typed as Iterable[str], but the function checks its truthiness using any((purls, ...)). In Python, generic iterators/generators (which are Iterable) always evaluate to True even if they are empty (i.e., yield no elements). Since purls is expected to be a collection (like a list or set) whose truthiness corresponds to its non-emptiness, we should type it as list[str] or Collection[str] to prevent potential correctness bugs and align with the actual usage.
| def is_a_wrapper(purls: Iterable[str], package_name: str, package_version_info: str | None) -> bool: | |
| """Check wrapper package properties.""" | |
| # if there's no purl and no package name or version, it's just wrapping element for | |
| # spdx package which is one layer below SPDXDocument in relationships | |
| return not any((purls, package_name, package_version_info)) | |
| def is_a_wrapper(purls: list[str], package_name: str, package_version_info: str | None) -> bool: | |
| """Check wrapper package properties.""" | |
| # if there's no purl and no package name or version, it's just wrapping element for | |
| # spdx package which is one layer below SPDXDocument in relationships | |
| return not any((purls, package_name, package_version_info)) |
1 participant
Over the past year to_cyclonedx() has grown significantly to accommodate various corner-cases. This change splits the function into several logically isolated chunks to keep it compact and easy to understand.