search

uber-go / config

Configuration for Go applications
https://godoc.org/go.uber.org/config
MIT License
448 stars 41 forks source link

Add lint and formatting harness #66

Closed ghost closed 7 years ago

ghost commented 7 years ago

Addresses https://github.com/uber-go/config/issues/63: Check for formatting and lint errors in CI Immodest copy paste of fx.

codecov[bot] commented 7 years ago

Codecov Report

Merging #66 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #66   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files          10       10           
  Lines         835      835           
=======================================
  Hits          803      803           
  Misses         22       22           
  Partials       10       10

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 196152f...2bce1aa. Read the comment docs.

akshayjshah commented 7 years ago

Mostly OOO for the next week and a half, resigning as reviewer.

drafnel commented 7 years ago

I happened to glance at the pull requests tab and saw this change. Just want to point out, in case you're unaware, that your merge should really be done the other way. You want the master branch on the left-hand side of the merge, and the thing you're merging into master on the right-hand side. So when you do the merge, you want to have the master branch checked out, and then do git merge $mybranch.

glibsm commented 7 years ago

Master merge was done to resolve the conflict, not to incorporate this code in (rebase is also an option, but requires a force push which often messes with the history of commit-comment timeline).

All the PRs get squash-merged back into master when they are approved.

drafnel commented 7 years ago

All the PRs get squash-merged back into master when they are approved.

Oh, yuck, why would you do that? The most important part of a commit is the commit message. So if someone uploads multiple commits for review they'll get squashed into one and the individual commits, along with their useful commit messages, will be thrown away like phabricator does? That's awful.

It seems like it is possible to address reviewer comments by updating the individual commits and force push to the side branch. At least that's what I've been doing. See 'git commit --fixup' and 'git rebase -i --autosquash' for the tools that make this possible/easy.

glibsm commented 7 years ago

I hear your comments, but the reality is that with GH, the squash merged experience is much better than the alternatives.

The pull request description becomes the background for the change (otherwise it's completely lost in the git log), not the little incremental commits that say "fix lint" or "remove a blank line".

It also results in a very nice git log which points to the PR that was merged, as well as uniformed git graph, without the intertwined intricacies of the merges.

It has worked very well for us in many projects, and there is a reason it has become a default merge option for the github PRs.

drafnel commented 7 years ago

What you want to encourage developers to do is to break their commits up on logical boundaries and give a thorough description about why each change is necessary and appropriate. That way the commit history is useful. This can be invaluable when trying to understand why some code is written the way it is. This information should be in the commit history, not on some external site.

If you're just going to throw all of that effort away, it removes the incentive for a developer to do it, and that's when you start getting terrible 3 word commit messages.

This practice of making additional commits (like "fix lint", "remove blank line", etc) to address reviewer comments, on top of the series you've uploaded is an awful and lazy practice (I'm pretty critical of the phabricator workflow). The right way to do this stuff is to amend the individual commits to address reviewer comments and then re-upload the series of commits. 'git commit --fixup' and 'git rebase --autosquash' make this workflow fairly easy.

I thought you guys were supposed to know what you're doing!!! :-)

@alsamylkin please don't squash my commits when you merge them.