5 ms·

Instead of using a service like this one (or Hound) that comments directly on pull requests, we chose to integrate the same tools in our build process with Phar

by remi 12y ago

Instead of using a service like this one (or Hound) that comments directly on pull requests, we chose to integrate the same tools in our build process with Phare (https://github.com/mirego/phare https://github.com/mirego/phare), a command-line tool we built which runs Rubocop, JSHint, JSCS, etc. and exits with a global status. If the code has coding style violations, the build breaks. Exactly like it would if we pushed a failing test. Personally, I’m not a fan of the noise generated by comments about coding style on pull requests.

hk__2 12y ago
I guess the advantage of Linthub over Phare is that it can run on pull requests made by external contributors.
- nl5887 12y ago
  This is exactly why we've develop this. To enforce code quality to pull requests and save the repository owners time.
  - bhuga 12y ago
    I think he's commenting on the approach of how the system reports, not its usefulness. Have you considered bundling up the results as a second commit status[1] rather than making comments? 1: https://github.com/blog/1935-see-results-from-all-pull-request-status-checks https://github.com/blog/1935-see-results-from-all-pull-reque...
    - jbrooksuk 12y ago
      Adding a commit status would be nice. It'd fail whilst the most recent commit has had a commit-comment on it.
      - nl5887 12y ago
        The commit status have been added :-).
        bhuga 12y ago
        That was quick. Awesome! Do you have a sample pull with the new setup?
- remi 12y ago
  Actually, it does in a certain way. Travis runs builds (tests + Phare) for all pull requests (eg. https://travis-ci.org/mirego/encore/pull_requests https://travis-ci.org/mirego/encore/pull_requests) so we know if pull requests made by external contributors "break the build".
  - nl5874 12y ago
    Yes, true, but only if it breaks. This enforces common code quality issues, so it will run side-to-side with travis.
    - ayrx 12y ago
      In all my projects (and the projects I contribute to), style issues are a build-breaking defect. In Python this usually means running flake8 as a Travis job.
cjbest 12y ago
+1 for "lint errors fail the build" If you also add "and any coding style things you care about, you have to make the linter check for", then you don't have to waste any time on that stuff in the pull request.
- sytse 12y ago
  GitLab B.V. CEO here. Totally agree that lint errors should fail the build instead of commenting. Otherwise all the people /cc't in the pull/merge request get all the hound comments in their email. And the nice thing about Rubocop https://github.com/bbatsov/rubocop https://github.com/bbatsov/rubocop is that you can disable tests easily. With GitLab we initially had: 406 files inspected, 1609 offenses detected. But after selecting only the most important checks and a day of fixing we got a green build.
nhaehnle 12y ago
There's an obvious trade-off here, though. For example, why stop at basic lint-style checks? You could make a static analyzer part of the build process and fail the build when a defect is detected. The trade-off is the same as with growing a test suite that always runs: Over time, your build times increase, and that makes development tedious. There's a balance to be found between core checks that run at build time, and additional, more extensive checks, which run automatically and asynchronously (!) once a developer believes their changes to be good enough. Both things have their place in a proper software development flow.
randx 12y ago
Same for me. I used Hound for a while but it was annoying to see all this comments. So I just added Rubocop check inside `rake test` so build is broken if you have code style issues

by remi 12y ago

hk__2 12y ago

I guess the advantage of Linthub over Phare is that it can run on pull requests made by external contributors.

nl5887 12y ago
This is exactly why we've develop this. To enforce code quality to pull requests and save the repository owners time.
- bhuga 12y ago
  I think he's commenting on the approach of how the system reports, not its usefulness. Have you considered bundling up the results as a second commit status[1] rather than making comments? 1: https://github.com/blog/1935-see-results-from-all-pull-request-status-checks https://github.com/blog/1935-see-results-from-all-pull-reque...
  - jbrooksuk 12y ago
    Adding a commit status would be nice. It'd fail whilst the most recent commit has had a commit-comment on it.
    - nl5887 12y ago
      The commit status have been added :-).
      - bhuga 12y ago
        That was quick. Awesome! Do you have a sample pull with the new setup?
remi 12y ago
Actually, it does in a certain way. Travis runs builds (tests + Phare) for all pull requests (eg. https://travis-ci.org/mirego/encore/pull_requests https://travis-ci.org/mirego/encore/pull_requests) so we know if pull requests made by external contributors "break the build".
- nl5874 12y ago
  Yes, true, but only if it breaks. This enforces common code quality issues, so it will run side-to-side with travis.
  - ayrx 12y ago
    In all my projects (and the projects I contribute to), style issues are a build-breaking defect. In Python this usually means running flake8 as a Travis job.

cjbest 12y ago

+1 for "lint errors fail the build" If you also add "and any coding style things you care about, you have to make the linter check for", then you don't have to waste any time on that stuff in the pull request.

sytse 12y ago
GitLab B.V. CEO here. Totally agree that lint errors should fail the build instead of commenting. Otherwise all the people /cc't in the pull/merge request get all the hound comments in their email. And the nice thing about Rubocop https://github.com/bbatsov/rubocop https://github.com/bbatsov/rubocop is that you can disable tests easily. With GitLab we initially had: 406 files inspected, 1609 offenses detected. But after selecting only the most important checks and a day of fixing we got a green build.

nhaehnle 12y ago

There's an obvious trade-off here, though. For example, why stop at basic lint-style checks? You could make a static analyzer part of the build process and fail the build when a defect is detected. The trade-off is the same as with growing a test suite that always runs: Over time, your build times increase, and that makes development tedious. There's a balance to be found between core checks that run at build time, and additional, more extensive checks, which run automatically and asynchronously (!) once a developer believes their changes to be good enough. Both things have their place in a proper software development flow.

randx 12y ago

Same for me. I used Hound for a while but it was annoying to see all this comments. So I just added Rubocop check inside `rake test` so build is broken if you have code style issues