On one of my pull requests for the Grand Comics Database project, Gemini Code Assist offered three suggestions: two warned about possible errors and the third suggested a clearer way to write a loop. I accepted the third and explained why I wasn’t making the other two changes. The warnings sounded reasonable, but they depended on conditions the code prevented.

In the pull request, I was reusing the site’s editing forms to check saved revisions before they advanced through its workflow. Data can reach a database without going through an editing form, so a successful save doesn’t establish that the form’s rules were followed. The AI review raised questions about missing values and whether every group of forms would get checked.

Three code review suggestions: two ruled out by existing checks and one accepted for clarity.

The first warning concerned Django formsets, which handle groups of related forms. The reviewer bot suggested guarding against missing minimum and maximum counts: converting Python’s None to the text "None" would produce a poor substitute for an integer. Fair enough, but where would those missing values come from? In Django 5.2’s formset factory, None is replaced with numeric defaults before the formset class is constructed, and the formsets in this validation path came through Django’s factories. By the time my helper received them, those limits were definitely numbers.

The second warning concerned an email recipient. The reviewer bot described a path where a change could enter the “In discussion” state without an assigned approver, then fail when the application tried to notify that nonexistent approver. It proposed having the notification helper quietly return when no recipient was supplied. But the transition to “In discussion” already rejected changes without an approver before it reached the notification call. Adding that guard would also let a future caller omit a required recipient without exposing the mistake. (The helper was later removed during further review; this was the code being reviewed at the time.)

One of the other project members then asked whether that restriction would prevent people from commenting on changes that hadn’t been assigned an approver. It wouldn’t: ordinary comments followed a separate path. That was a useful question because “put into discussion” and “add a comment” sound much closer in ordinary English than they were in the application’s workflow. Explaining the rejected suggestion also gave someone familiar with the site’s use a chance to check its consequences.

The suggestion I accepted concerned this line inside the validation loop:

valid = formset.is_valid() and valid

Python evaluates the left side first, so each formset gets checked even if an earlier check has already failed. If you swap the operands, valid and formset.is_valid() would skip the call once valid is false. The original order was deliberate: the contributor should get all the validation errors in one pass. But in my implementation, it was also an easy detail for a future edit to miss.

The suggested replacement made that intention explicit:

if not formset.is_valid():
    valid = False

The surrounding loop still checks every formset, and a failed check keeps the overall result false. I accepted the change for readability and future maintenance, with the reviewer bot credited in the commit. Working code can be worth improving. It doesn’t need an imaginary bug to justify the edit.

For each suggestion, ask what has to be true for it to apply. Follow the value back to where it’s created, and follow the caller through the checks that run first. If the claimed failure is reachable, reproduce it and preserve a test for it. If an earlier check rules it out, explain which check and why. Evaluate a clarity improvement on how well it communicates the intended behavior.

The PR merged into beta on September 24 after further review. That review also narrowed validation to submission, allowing editors to approve intentional backend corrections that the editing forms couldn’t express. The final change reflected more than the three AI suggestions. Recording why each suggestion was accepted or rejected left the human reviewers something they could examine and challenge.

—jhunterj

Join the conversation

Your email address will not be published. Required fields are marked *