search

dgraph-io / dgraph

The high-performance database for modern applications
https://dgraph.io
Other
20.41k stars 1.5k forks source link

Code Review System #97

Closed manishrjain closed 7 years ago

manishrjain commented 8 years ago

We need a code review system which can highlight all the golint, go vet, and other issues. Given the high noise ratio, we want to show these to the reviewer and the author of the change, so they can make a decision about which ones to fix. Not all these issues are relevant, so we don't want to block the changelist just because golint complains.

So far I think Phabricator seems to have such a mechanism. We should also look into Gerrit. Once decided, we should set up one. Suggestions on other code review systems which can allow for above customization are welcome!

sreeram-dev commented 8 years ago

At HackerEarth, We use phabricator. It is working very well for a mid-size team. Features: Code Review Wiki Hosting Manifest for triaging tasks.

you can host it yourself.

hackintoshrao commented 8 years ago

Integration of govet, gofmt and other tool sets in Travis would solve the need too.

manishrjain commented 8 years ago

@sreeram-boyapati Would it be able to show errors from a customized golint? We want to do it so it can show the author and the reviewer these things if needed. So, the reviewer can decide which ones the author should fix, and which ones to ignore.

@hackintoshrao Travis would come into the picture after the code has already been pushed to master. We need this during code review time.

sreeram-dev commented 8 years ago

@manishrjain https://github.com/kalbasit/arcanist-go https://secure.phabricator.com/T6867

We use arc as a command line tool to push commits. we can integrate go linters so the code pushed for review is written properly. It has not yet been accepted into the main repo though.

dancompton commented 8 years ago

Along the lines of what @hackintoshrao said: Seems natural to have this run by a build container (to eliminate inconsistency) and have every branch run tests -- not just master. There's a wercker build-step much like what you describe: https://github.com/wercker/step-golint

I'm not a wercker expert, but it might be possible to have wercker publish a comment on the PR via webhook which contains the formatted output. Although this seems redundant given that the test output will contain this information.

manishrjain commented 7 years ago

Reviewable is working out alright for us. We'll probably write a Github bot which can do some of the special things that we need done. Closing this issue.