feat: log which requirements files are used during pip dependency resolution#1419
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds logging to show which requirements files are being used for dependency resolution, which is a helpful improvement for visibility. My review includes a suggestion to refactor the new logging code to reduce duplication and improve maintainability, in accordance with the repository's style guide.
| log.info("Using requirements files: %s", [str(f.subpath_from_root) for f in resolved_req_files]) | ||
|
|
||
| resolved_build_req_files = resolve_req_files(build_requirement_files, True) | ||
| log.info( | ||
| "Using build requirements files: %s", | ||
| [str(f.subpath_from_root) for f in resolved_build_req_files], | ||
| ) |
There was a problem hiding this comment.
There's some code duplication in these logging statements, and the formatting is inconsistent between them. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the logging logic into a small helper function within _resolve_pip.
For example:
def _log_resolved_files(description: str, files: list[RootedPath]):
log.info("Using %s: %s", description, [str(f.subpath_from_root) for f in files])
# Then call it:
resolved_req_files = resolve_req_files(requirement_files, False)
_log_resolved_files("requirements files", resolved_req_files)
resolved_build_req_files = resolve_req_files(build_requirement_files, True)
_log_resolved_files("build requirements files", resolved_build_req_files)This would resolve both the duplication and the inconsistent formatting, making the code cleaner and easier to modify in the future.
References
- The repository style guide recommends detecting and flagging code duplication. The logging logic for requirements files and build requirements files is duplicated. (link)
e115ad0 to
b43a078
Compare
b43a078 to
66183ff
Compare
| "Using requirements files: %s", [str(f.subpath_from_root) for f in resolved_req_files] | ||
| ) | ||
|
|
||
| resolved_build_req_files = resolve_req_files(build_requirement_files, True) |
There was a problem hiding this comment.
Why did you decide to drop the check?
There was a problem hiding this comment.
that condition is leftover from an earlier version where I was warning for both runtime and build requirements. I felt there no need to warn against empty build requirements , but forgot to clean up . for dropped check , resolve_req_files() already handles it, when requirement_files is None and the default file does not
exist it returns an empty list, so checking not resolved_req_files alone is sufficient.
| log.info( | ||
| "Using build requirements files: %s", | ||
| [str(f.subpath_from_root) for f in resolved_build_req_files], | ||
| ) |
There was a problem hiding this comment.
I felt there no need to warn against empty build requirements.
I agree with not warning about it, but I'd still log a different message, at an info level, when no build requirements files are found. I feel like it's weird to log an empty list.
There was a problem hiding this comment.
When a user specifies requirements files explicitly (e.g. requirements-extra.txt), the default requirements files are skipped. Am I just overthinking scenarios here, or is this a case worth handling?
There was a problem hiding this comment.
I think this is the intended behavior. When the user wants only requirements-extra.txt, he can specify that. If he wants requirements-extra.txt and the default one, he would have to specify both. In both cases you logic will only report the specified files, which is IMO correct.
When no requirements files are resolved, a warning is emitted since this likely indicates a misconfiguration. When files are found, an info log lists the resolved paths. For build requirements, a missing file is logged at info level as it is less critical than runtime dependencies. Resolves hermetoproject#1418 Signed-off-by: Harshit Bansal <harshitbansal184507@gmail.com>
66183ff to
42df51c
Compare
Log the resolved requirements and build requirements files at the start
of dependency resolution so it is immediately visible when no files
were found.
Resolves #1418