5 ms·

Code style and “are tests passing” should not even be there. Code style should be automated same as running tests when someone creates PR.

by ozim 3y ago

hakre 3y ago
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 3y ago
  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 3y ago
    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.
tgv 3y ago
I don't care about code style. Granted, I work in a small team, but I work on a project with someone with a different style. All I ask is that we respect each other's style. And that he puts braces around single statement branches. What's surprising to me in the article is the pyramid style. In all business school and psychology texts, the top of the pyramid is the most important one. Here it's the least important.
- ozim 3y ago
  There should be no "someone with other style working on project" - project should have its style and dev should adjust to that style. Same with joining new team, one might have preferences but you leave them at the door and format code with whatever already is there. There is tooling where you can also have formatting configuration per repo so if you use IDE it would take that config and format it for you in "project style".
  - tgv 3y ago
    I do not agree. Everyone can have their own style, as long as it's consistent enough and readable.
    - ozim 3y ago
      I consider code and project property of a company I work for. If they have dress code or code style it is for them to set it up. I might propose some changes that fit me but most of the time "when in Rome do as the Romans do".
      - tgv 3y ago
        Sure, but we are the Romans. The code base was a shambles. At least things runs without failing now. And management considers features and selling more important than maintenance, so the best we can do is keep it simple. > If they have dress code or code style it is for them to set it up. I also chose this company because they don't. A dress code, what power-hungry lunacy is that?
    - 3y ago
jeremyjh 3y ago
Formatting is not the only component of code style, it's just the part that can easily be automated. Anytime there is more than one way to do something, and both options are correct and produce the same result, the choice between them is likely to be a matter of taste or style. For example, using a loop instead of using a map function. You can automate some of that stuff, but never all of it.

Code style and “are tests passing” should not even be there. Code style should be automated same as running tests when someone creates PR.

by ozim 3y ago

Code style and “are tests passing” should not even be there. Code style should be automated same as running tests when someone creates PR.

hakre 3y ago

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 3y ago
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 3y ago
  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.

tgv 3y ago

I don't care about code style. Granted, I work in a small team, but I work on a project with someone with a different style. All I ask is that we respect each other's style. And that he puts braces around single statement branches. What's surprising to me in the article is the pyramid style. In all business school and psychology texts, the top of the pyramid is the most important one. Here it's the least important.

ozim 3y ago
There should be no "someone with other style working on project" - project should have its style and dev should adjust to that style. Same with joining new team, one might have preferences but you leave them at the door and format code with whatever already is there. There is tooling where you can also have formatting configuration per repo so if you use IDE it would take that config and format it for you in "project style".
- tgv 3y ago
  I do not agree. Everyone can have their own style, as long as it's consistent enough and readable.
  - ozim 3y ago
    I consider code and project property of a company I work for. If they have dress code or code style it is for them to set it up. I might propose some changes that fit me but most of the time "when in Rome do as the Romans do".
    - tgv 3y ago
      Sure, but we are the Romans. The code base was a shambles. At least things runs without failing now. And management considers features and selling more important than maintenance, so the best we can do is keep it simple. > If they have dress code or code style it is for them to set it up. I also chose this company because they don't. A dress code, what power-hungry lunacy is that?
  - 3y ago

jeremyjh 3y ago

Formatting is not the only component of code style, it's just the part that can easily be automated. Anytime there is more than one way to do something, and both options are correct and produce the same result, the choice between them is likely to be a matter of taste or style. For example, using a loop instead of using a map function. You can automate some of that stuff, but never all of it.