Yet more virtual league detail

[PeterJCLaw]

See also srobo/docs#661

[PeterJCLaw]

Meta: it's fairly unusual to push commits (edit: including applying review suggestions) to a PR for which you are not the author without prior coordination of the author.

[samjmartin04]

Meta: it's fairly unusual to push commits to a PR for which you are not the author without prior coordination of the author.

This was due to GitHub not allowing me (as far as I'm aware) to add suggestions for changing the "providing logs" sentence as it was more than a few lines outside of the rest of the changes in this PR.

[PeterJCLaw]

Continuing the meta discussion for knowledge sharing. This comment is therefore deliberately detailed, aiming to share the full context of experience for educational purposes. The length of this comment is not proportional to the concern.

This was due to GitHub not allowing me (as far as I'm aware) to add suggestions for changing the "providing logs" sentence as it was more than a few lines outside of the rest of the changes in this PR.

With regards to that UI

I agree it's unfortunate that GitHub doesn't make it easy to add comments outside the bounds of the diff. There's a trade-off here. On the one hand, encouraging review to focus on the things which have changed and not on generically improving the rest of the content is a good thing. Small focussed changes are generally better than bigger broader ones as they're easy to review. This applies just as much to prose as it does to code. On the other hand, it can easily be the case that an update in one place suggests or requires an update elsewhere to be made too. (I'm not sure if that's the case here, I haven't looked) The common response to such scenarios is to post on the part of the change which has been made something like "I think we should also change X in file Y on line Z". The author can then determine what changes, if any, need to be done. (I have encountered cases both as reviewer and as author where not changing the "other" place was correct)

In case you're not aware, there is also a way to have comments associated with files and not with lines in that file. This is useful in various scenarios, including the one described above. It can also be used to suggest related general improvements, however I would caution about scope creep of changesets.

With regards to the action

Perhaps I should clarify that by "pushing commits" I meant generically making changes to the content of the PR (which is naturally all expressed via commits). So this includes things like applying suggestions from other reviewers as well as writing commits manually and pushing them. It probably also includes rebasing someone else's PR or merging in the main branch.

Even if well-meaning, all of these things are likely to surprise the author and create work for them to catch up on changes that have been made by someone else to something they were working on. It can be taken as disrespect for the author. (I can imagine that in some communities there are different norms, however what I'm describing here is the general norm on GitHub and, as far as I'm aware, one which SR follows too).

It can also lead to duplication of effort -- for example if they had been working on alternative versions of the same fixes. While perhaps less likely for prose content, for source code it is likely that the author has additional context about the change which the reviewers might not have and which could suggest quite different changes.

Even if well-meaning and conflicting duplication of effort is avoided, it is unlikely to reduce the workload of the author and is likely to increase it. This is because, unless the PR is fully handed over to whoever has pushed to the PR, the author is still the one responsible for merging it. In doing that they are still responsible for its content and thus must still read the review comments and evaluate the proper course of action to resolve them. Where changes have been made on top of their own work, they then need to review those changes¹ to determine their appropriateness (and potentially make further fixes).

In the ideal case, code review is a discussion between multiple parties all trying to produce the best outcome. (Along the way the reviewer(s), author or both hopefully also learn some things to improve their craft.) Trust is required in both directions. Authors are usually encouraged to defer to the judgement of reviewers in cases which are ambiguous, broadly equivalent or especially in matters relating to e.g: clarity. Reviewers in turn are expected to offer their suggestions for the consideration of the author, rather than to force a particular response.

Footnotes

1. Authoring and reviewing content are different modes of work², hence switching between reading review comments and reviewing changes made are different and this therefore creates a context switch. ↩

2. This is one of the reasons that generative code tooling results in poor output -- it removes the "author" mode from the code development cycle and replaces it with a second reviewer. Humans are bad reviewers (but mostly decent authors), resulting in lower overall quality. (see also https://qntm.org/assist) ↩

[ShadowLord2005]

All looks good to me