I hear your frustrations.
Let's just call them "unproductive" reviews.
A reviewer is trying to establish two things about code
before we merge it to
- Is it correct?
- Is it maintainable?
Beyond that, maybe the Author taught the Reviewer something
by sharing the code.
Often the Reviewer will seek to teach the Author,
about concepts that may provoke edits to this PR
but usually will influence how Author writes future code.
It is seldom that I encounter a PR with an outright bug
on which I must write an imperative "Fix this!"
Most of my advice comes across in a gentler tone:
Doing X was odd because of .... Consider doing Y. Prefer Z.
For such discretionary items, Author always gets final say.
Having viewed the code from another person's perspective,
you might agree and decide to make an edit,
or feel it doesn't rise to that level and just merge,
with no hard feelings.
When Author submits short PRs, they are easy
to approve and quickly get through the process.
Larger PRs OTOH create additional work for reviewers,
even if three small ones show up combined as one big PR.
Because now the Reviewer has to pick his battles,
choosing and prioritizing the most egregious issues.
This requires more effort than just blurting out
each issue you see as you read down the list of edits.
Also, big PRs tend to create greater cognitive load
due to more forward references.
If you're making a mechanical "whitespace change"
or "rename of popular function" that touches a
bunch of files, do go to the trouble of breaking
that out as an initial PR, and then a subsequent PR
can build new features upon that groundwork.
This should just not be a thing.
You shouldn't be bringing it up,
your reviewer shouldn't be making remarks about it,
it should just not happen.
Life is too short.
We have features to implement.
Your team (or project) should have an on-boarding
document that writes down policies like "lint your code!",
describing the expectations up front,
including particular tools and settings.
Auto-formatting of source code works well for some teams.
Sorting of imports is work for the computer to do, not humans.
If your code doesn't lint clean (e.g.
$ make lint),
then that's a code defect and the code wasn't ready
to begin the PR process much less being ready for merging.
A reviewer should never be raising lint issues
during review. If that happens, it indicates a process problem.
Inventing good, accurate names is surprisingly hard.
Sometimes you'll come back to yesterday's code
and say "what was I thinking?!?", and ask the IDE
to globally rename some unhelpfully vague identifier.
Reviewers add value by proposing name improvements.
Author doesn't have to accept all suggestions.
If any review remarks are show stoppers,
they should be addressed before merge,
or minimally they should spawn new issue(s)
to be worked on soon and be approved in a subsequent PR.
Any other review remarks should not hold up approval.
Author can agree or not, make edits or not, and merge it down.