[hakre]

Your relatively short comment caught my interest and I'd like to learn more about your thoughts.

As, IMHO it depends (as so often). Code review is commonly the place that puts the current style under test (hence automated otherwise you don't have the results during review) and under review.

E.g. Code review finds out if a code-style is missing, incomplete or even outright wrong.

If that approach is taken, passing tests and code-style must be in there, otherwise it is not useful to argue about violations. From my understanding I would therefore see those results be available during review.

It may also be that there is code-style for automated test-code, and that code part of the review. Without taken this into context of the code-review, I miss a bit the boundary of your comment.

Could you elaborate a bit where you draw the line and why? E.g. I can imagine there is a benefit to keep a distinct context for code-reviews so that they are still practically feasible and those parts that need further adoption are put into a different phase, like steps before (preparation of the current increment) or after (preparation of next increments) the code-review itself.

[ozim]

Style formatting should be automated on level of IDE or other CI tooling.

Discussing or fixing code style is huge waste of time and styling can be automated. Yes it will be broken in some edge cases but for me that is acceptable because my goal is to deliver the feature and not to make "perfectly aligned code". Aligning code by hand is waste of time, thinking about aligning code is waste of time. Formatting should be in configuration and you format whole file at once and never do stuff by hand.

Passing tests is also easy to automate when creating PR, CI should run tests and tell that to person creating PR - hey tests failed, fix it and then after they pass you can open PR, not a fellow developer because that is waste of time for everyone.

I did not write "tests should not be there" only "are all tests passing?" is not part of code review for me, it is something that is there before code review even begins.

So checking if there are proper tests and if tests make sense is part of code review.

[hakre]

Thanks, that better helped me to understand your comment. Additionally I was not aware if that was with pull-requests or more with regular code-reviews as part of the development process. That was an additional insight, thanks for sharing.

Nevertheless, things can be different, e.g. if CI passes, it merges already, no need to have a request for merge, which are often a blocker to keep a steady development flow. Naturally, this benefits from tests that are already run during development, if not driving the development.

And I didn't read you're against testing nor aligning white space and comments at all, more the localization. Actually the points you raise are looking important to me, because if alignment comes in late, this can cause a lot of erosion, which can hinder any review if not even provoke merge conflicts which are stopping the process quite early and require re-iteration.

Your approach should also work towards non-release blocking code reviews, a property I personally like, as I've seen teams struggle with code reviews, becoming more and more of a burden, even after practicing it has found its way. But that is only a subjective comment of mine, every project is different, which makes it an interesting topic for me.