docs: add docstrings and comments to complex functions by LalatenduMohanty · Pull Request #1047 · python-wheel-build/fromager

LalatenduMohanty · 2026-04-13T02:19:11Z

Add docstrings to functions and inline comments to non-obvious code patterns across various files. Only targets genuinely complex logic that would be hard for new contributors to understand.

Though my goal was to help contributors but it has also a positive impact on AI code assistants. When an AI assistant needs to understand a function, it either reads the docstring or reads the entire function body. A good docstring lets it understand the function's purpose and behavior without consuming context window on implementation details.

Closes: #1046

coderabbitai · 2026-04-13T02:19:27Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9038197b-c35b-4766-b6c4-36c63a258af6

📥 Commits

Reviewing files that changed from the base of the PR and between 3c2ad0f and 2200c13.

📒 Files selected for processing (1)

src/fromager/bootstrapper.py

✅ Files skipped from review due to trivial changes (1)

src/fromager/bootstrapper.py

📝 Walkthrough

Walkthrough

This PR adds docstrings and inline comments across many src/fromager modules (finders, resolver, bootstrapper, wheels, sources, dependencies, pyproject, external_commands, requirements_file, dependency_graph, vendor_rust). It also makes three behavioral changes: bootstrapper now re-extracts a wheel’s build tag and rejects non-matching wheels, dependency filtering passes the parent requirement’s extras into marker evaluation, and requirements_file evaluates markers using a separate marker environment per extra value. No public signatures were changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	❓ Inconclusive	All changes are documentation additions (docstrings and inline comments) with no functional logic modifications. One minor functional change in bootstrapper.py's _look_for_existing_wheel adds wheel metadata validation logic not explicitly listed in `#1046`.	Clarify whether the wheel build_tag validation logic in bootstrapper.py is part of issue `#1046` or represents an out-of-scope functional change that should be separated into a distinct PR.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding docstrings and comments to complex functions across the codebase.
Description check	✅ Passed	The description is directly related to the changeset, explaining the purpose of adding docstrings and comments to complex functions and mentioning benefits for contributors and AI assistants.
Linked Issues check	✅ Passed	The PR comprehensively addresses all 12 functions specified in issue `#1046` with appropriate docstrings and inline comments covering git URL parsing [`#1046`], multi-stage filtering [`#1046`], naming fallback strategies [`#1046`], wheel metadata handling [`#1046`], retry/temp file logic [`#1046`], environment variable merging [`#1046`], and deduplication algorithms [`#1046`].

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/fromager/bootstrapper.py (1)

1081-1089: ⚠️ Potential issue | 🟠 Major

Handle invalid cached wheel filenames as cache misses.

At Line 1084, a parse failure from extract_info_from_wheel_file() can abort bootstrap during cache lookup. This path should degrade to “not cached” and continue.

Proposed fix

-        _, _, build_tag, _ = wheels.extract_info_from_wheel_file(req, wheel_filename)
+        try:
+            _, _, build_tag, _ = wheels.extract_info_from_wheel_file(
+                req, wheel_filename
+            )
+        except ValueError as err:
+            logger.info(
+                "ignoring cached artifact with invalid wheel filename %s: %s",
+                wheel_filename,
+                err,
+            )
+            return None, None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fromager/bootstrapper.py` around lines 1081 - 1089, The call to
wheels.extract_info_from_wheel_file(req, wheel_filename) may raise a parse error
and currently can abort bootstrap; wrap that call in a try/except that catches
the parse/validation exception (e.g., ValueError or the specific exception
raised by extract_info_from_wheel_file) and treat it as a cache miss by logging
a debug/info message and returning (None, None) so execution continues; update
the logic around extract_info_from_wheel_file and the subsequent
expected_build_tag check to rely on the safe, exception-handled result.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/fromager/bootstrapper.py`:
- Around line 1081-1089: The call to wheels.extract_info_from_wheel_file(req,
wheel_filename) may raise a parse error and currently can abort bootstrap; wrap
that call in a try/except that catches the parse/validation exception (e.g.,
ValueError or the specific exception raised by extract_info_from_wheel_file) and
treat it as a cache miss by logging a debug/info message and returning (None,
None) so execution continues; update the logic around
extract_info_from_wheel_file and the subsequent expected_build_tag check to rely
on the safe, exception-handled result.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cc6ee430-a315-4dbc-8dac-78e8d6cb8470

📥 Commits

Reviewing files that changed from the base of the PR and between b8f4441 and 14113ea.

📒 Files selected for processing (11)

src/fromager/bootstrapper.py
src/fromager/dependencies.py
src/fromager/dependency_graph.py
src/fromager/external_commands.py
src/fromager/finders.py
src/fromager/pyproject.py
src/fromager/requirements_file.py
src/fromager/resolver.py
src/fromager/sources.py
src/fromager/vendor_rust.py
src/fromager/wheels.py

Add docstrings to functions and inline comments to non-obvious code patterns across various files. Only targets genuinely complex logic that would be hard for new contributors to understand. Closes: python-wheel-build#1046 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

jskladan

Obviously, all of these are nitpicks, but since we're talking docs, they feel appropriate.

I see nothing wrong with the MR as-is, so hitting that Approve button (FWIW), and leaving it up to you.

jskladan · 2026-04-14T12:33:05Z

        visited: set[str],
    ) -> typing.Iterable[DependencyEdge]:
+        # Depth-first walk of install requirement edges, skipping nodes
+        # already visited to avoid loops in the dependency graph.


Not that it does any harm, but the comment describes pretty self-explanatory code, while not adding any important context/information in the same fashion the other comments do. There is no (non-obvious) "why" explained.

jskladan · 2026-04-14T12:34:41Z

        graph = cls()
+        # Stack-based DFS to reconstruct graph from serialized dict,
+        # skipping nodes already visited to avoid processing shared
+        # dependencies twice.


This feels like it could have been a docstring instead, if you want to keep it in? Othervise, also feels that it does not add much value other than describing "what" code does (and we have the code to describe what it does already :)).

jskladan · 2026-04-14T12:35:20Z

        raise NotImplementedError("GenericProvider does not implement caching")

    def find_candidates(self, identifier: typing.Any) -> Candidates:
+        # Accepts three input formats from _version_source:


How about we make this a docstring instead?

rd4398

This looks good to me. I will wait for Lala to address comments on PR

LalatenduMohanty requested a review from a team as a code owner April 13, 2026 02:19

LalatenduMohanty marked this pull request as draft April 13, 2026 02:19

LalatenduMohanty force-pushed the add_docstrings_non_trivial_code branch from 14113ea to 38f90a7 Compare April 13, 2026 02:20

coderabbitai bot reviewed Apr 13, 2026

View reviewed changes

LalatenduMohanty force-pushed the add_docstrings_non_trivial_code branch 2 times, most recently from 2b46652 to cee17ce Compare April 13, 2026 02:32

LalatenduMohanty force-pushed the add_docstrings_non_trivial_code branch from cee17ce to 3c2ad0f Compare April 13, 2026 02:35

LalatenduMohanty marked this pull request as ready for review April 13, 2026 02:40

jskladan approved these changes Apr 14, 2026

View reviewed changes

rd4398 approved these changes Apr 15, 2026

View reviewed changes

Merge branch 'main' into add_docstrings_non_trivial_code

2200c13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add docstrings and comments to complex functions#1047

docs: add docstrings and comments to complex functions#1047
LalatenduMohanty wants to merge 2 commits intopython-wheel-build:mainfrom
LalatenduMohanty:add_docstrings_non_trivial_code

LalatenduMohanty commented Apr 13, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 13, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

❌ Failed checks (1 inconclusive)

Uh oh!

coderabbitai bot left a comment

Uh oh!

jskladan left a comment

Uh oh!

jskladan Apr 14, 2026

Uh oh!

jskladan Apr 14, 2026

Uh oh!

jskladan Apr 14, 2026

Uh oh!

rd4398 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

LalatenduMohanty commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

❌ Failed checks (1 inconclusive)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

jskladan left a comment

Choose a reason for hiding this comment

Uh oh!

jskladan Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

jskladan Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

jskladan Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

rd4398 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LalatenduMohanty commented Apr 13, 2026 •

edited

Loading

coderabbitai bot commented Apr 13, 2026 •

edited

Loading