search

bookio / server-old

2 stars 0 forks source link

Code review #11

Open javidgon opened 10 years ago

javidgon commented 10 years ago

What about implementing "code review" before merging a Pull Request? This way bugs and other problems might be spotted before they get into production.

Based in my experience it also improves dramatically the quality of the final product.

(Not sure if it exists already, if so, please feel free to close this issue)

magnus-engstrom commented 10 years ago

:+1:

This is also a easy way of keeping code in projects from being "private property".

I would recommend doing code reviews as a group effort, with the team joining over Skype och Google Hangout. There can also be a chosen theme for every session, like "improve testing" or "stripping controllers".

javidgon commented 10 years ago

@mageng The biggest problem of "global reviewing sessions" (at least in my opinion) it's the need for matching schedules among team members, which it's not an easy thing. Said that, we could try out the normal "code review" way, and then we can improve it later on if necessary

mounte commented 10 years ago

Also we should write down guidelines for coding in the project. We should links to appropriate style guides for Ruby etc.

I always try not to commit to master since I want to work with pull requests so that everyone can discuss and improve I think that is good practice. I dont know what the permissions look like if we should have gatekeeper for merging pull requests etc.

I dont know how many of you have worked with gitflow as workflow for git projects, please check out: https://www.atlassian.com/git/workflows#!workflow-gitflow and http://nvie.com/posts/a-successful-git-branching-model/ I think it is a promising strategy and worth considering.

joakimbengtson commented 10 years ago

I agree with @javidgon about global reviews. Maybe something more like pair programming? You have a partner (rotating) that reviews a pull request?

But you are far more experienced with working with a distributed team in github than me and @meg768, so it´s up to you to decide a model to try... ;)

joakimbengtson commented 10 years ago

@mounte guidelines are good, but we should keep it very short and very simple. I've been to endless meetings with programmers discussing guidelines down to the number of spaces used for indentation and naming conventions ;)

Is there anything we can "steal" from some popular open source project?

javidgon commented 10 years ago

I agree with both @joakimbengtson @mounte. I think we need some kind of guidelines but just in certain parts. For example, as a suggestion, I'd like to propose to use some guidelines for commits (see e.g http://stackoverflow.com/questions/43598/suggestions-for-a-good-commit-message-format-guideline).

Regarding "code review", I think we should try to keep it as "informal" as possible. With this I mean that we shouldn't oblige a certain person to review a PR, but to require at least one :+1: in each PR (without specifying who should do it). The main reason for this: Not everyone is an expert on a certain technology/language, so a person should review a PR only if he feels capable to.