4 ms·

> Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. What if you post a PR and forgot to ru

> Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. What if you post a PR and forgot to run a linter or configure your editor? Should that not justify a comment over style violations? The problem will only go away if everyone is on the same page.

philwelch 2y ago
CI should reject the feature branch if the linter fails. Never waste an engineer’s time doing work a program can do. It’s also often handy to configure linters and autoformatters as precommit hooks.
- chipdart 2y ago
  > CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.
  - nemetroid 2y ago
    Just make sure that the pipeline is fast, or allow an override, and there is no issue.
    - chipdart 2y ago
      Requiring manual intervention to handle a blocked pipeline over a non-issue defeats the whole purpose of continuous integration, not to mention that you are suggesting adding twice the complexity as an alternative to not adding any complexity at all. And should I stress again that there is absolutely zero positive tradeoffs?
      - nemetroid 2y ago
        > And should I stress again that there is absolutely zero positive tradeoffs? Just because you don't value or acknowledge them doesn't mean they don't exist.
      - philwelch 2y ago
        Who’s blocking any pipelines here? If you’re running a PR process at all, these changes are being pushed to a feature branch and reviewed before getting merged to master. If you have a feature branch that’s failing CI, that doesn’t block me from merging my feature branch once my PR is approved. As I specifically said, the CI should fail on the feature branch. If you’re only running CI on master, you’re going to run into the exact same problem if your unit tests fail. The way to avoid that problem is to run the unit tests on your feature branch, and if you’re doing that you might as well run the linters too.
  - thiht 2y ago
    > Your CI pipeline is broken if it refuses to run because of style issues. Strong disagree. If it’s not in the CI, it’s optional. Respecting the formatting standards of the project is NOT optional. It should definitely be applied as a pre-commit or pre-push hook too to make sure the CI step is just a formality, but it’s not enough.
  - philwelch 2y ago
    > Your CI pipeline is broken if it refuses to run because of style issues. The whole point of CI is to automatically verify the code. Linters are a method of doing that, the same as tests. > Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4. Sounds like an easy fix to me. And if your CI is set up properly, the misformatted branch wouldn’t have been merged to master in the first place.
    - chipdart 2y ago
      > The whole point of CI is to automatically verify the code. Against errors and regressions. Meaning, stuff that breaks your code and affects the service you provide to users. Style issues ain't that. Come on.
      - philwelch 2y ago
        Linters aren’t only for checking style issues.

philwelch 2y ago

CI should reject the feature branch if the linter fails. Never waste an engineer’s time doing work a program can do. It’s also often handy to configure linters and autoformatters as precommit hooks.

chipdart 2y ago
> CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.
- nemetroid 2y ago
  Just make sure that the pipeline is fast, or allow an override, and there is no issue.
  - chipdart 2y ago
    Requiring manual intervention to handle a blocked pipeline over a non-issue defeats the whole purpose of continuous integration, not to mention that you are suggesting adding twice the complexity as an alternative to not adding any complexity at all. And should I stress again that there is absolutely zero positive tradeoffs?
    - nemetroid 2y ago
      > And should I stress again that there is absolutely zero positive tradeoffs? Just because you don't value or acknowledge them doesn't mean they don't exist.
    - philwelch 2y ago
      Who’s blocking any pipelines here? If you’re running a PR process at all, these changes are being pushed to a feature branch and reviewed before getting merged to master. If you have a feature branch that’s failing CI, that doesn’t block me from merging my feature branch once my PR is approved. As I specifically said, the CI should fail on the feature branch. If you’re only running CI on master, you’re going to run into the exact same problem if your unit tests fail. The way to avoid that problem is to run the unit tests on your feature branch, and if you’re doing that you might as well run the linters too.
- thiht 2y ago
  > Your CI pipeline is broken if it refuses to run because of style issues. Strong disagree. If it’s not in the CI, it’s optional. Respecting the formatting standards of the project is NOT optional. It should definitely be applied as a pre-commit or pre-push hook too to make sure the CI step is just a formality, but it’s not enough.
- philwelch 2y ago
  > Your CI pipeline is broken if it refuses to run because of style issues. The whole point of CI is to automatically verify the code. Linters are a method of doing that, the same as tests. > Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4. Sounds like an easy fix to me. And if your CI is set up properly, the misformatted branch wouldn’t have been merged to master in the first place.
  - chipdart 2y ago
    > The whole point of CI is to automatically verify the code. Against errors and regressions. Meaning, stuff that breaks your code and affects the service you provide to users. Style issues ain't that. Come on.
    - philwelch 2y ago
      Linters aren’t only for checking style issues.