Attention is currently required from: Felix Singer, Nico Huber, Angel Pons.

Patch set 13:Code-Review +2

View Change

1 comment:

• File ichspi.c:

  • Patch Set #7, Line 872: write_cmd = true;

    Thanks Angel for your comment! You summarized the situation very good. […]

    As a reviewer who went through the whole chain, I can say IMO:

    1) It was easier to review as a chain of smaller patches (as it is now) VS one gigantic patch. I appreciate patch owner did that work. Thank you!

    2) My understanding that changing the type int -> bool changes the binary, so commits are not reproducible, doesn't matter whether changes are split or combined.

    3) Renaming only comes up for this one file (two variables in here), not for every patch in the chain. I read this comment on my first round of review and payed attention to variables names. I didn't notice anything obvious to rename.
    Which means, in this specific situation, renaming in a separate patch does not mean twice more patches, it means one more patch.

    4) In addition to 1), focusing the chain on one thing, "retyping to bool" makes it easier to review.

    5) Changing this one patch now to do two things (retype and rename) unfortunately will create more work for patch owner and reviewers, because this version is already rebased, reviewed and tested.

    The best thing: everyone has the same end goal in mind: "bool variables with meaningful names" ! :) As I said earlier, I agree with renaming `is_write_command`, it's a good idea.

To view, visit change 66892. To unsubscribe, or for help writing mail filters, visit settings.