Closed MajorBreakfast closed 8 years ago
Don't merge just yet. There seems to be a problem with the test glob pattern. Resolving now...
Ok done
@WoH I yield to thee the code review honours once more
@philbu I refactored your code. Plz have also a look at lib/main
(formerly server.js
)
Looks good to me, can only comment Mocha / Chai though.
Everything seems fine to me. Should I click on this big green "Merge pull request" button or is there a voting system or sth like that? (Don't hit me please, I am not used to github and its processes ;) )
@philbu You withstood tempation of clicking the button :) That's good. I'd say that generally it's best to merge PRs fast, but not too fast either because more eyes are always better.
BTW since it seems that you're new to PRs:
git remote set-url origin <some url>
can be used to change the "origin" remote's url to your own repo as it's best to push changes to your own fork on your github account first. Then you create a PR on github once it's ready so that we can see it.
Example: git remote set-url origin https://github.com/MajorBreakfast/some-repo
Note: Use git remote -v
to see what the current url is
git remote add main-repo <https://gith...>
and then pull from our main repo's "develop" branch when things update while you're developing via git pull main-repo develop
.@philbu and I'll only add: Unfortunately there's no consensus in GitHub, but I would do it like this:
@MajorBreakfast Yes, PRs should extinguish between 2-3 days. If it takes many commits / much time to fix a PR, close it and then open it again once all the fixes have all been implemented and it's ready to be reviewed again.
also: you forgot to assign people here -->
Assigning people to a PR is important because you can keep track of who read it. Also it's ok to assign only one other team member to a PR, it will then be his/her responsibility to judge if another reviewer is needed or not (like the points above).
Last but not least for @PokemonGoers/catch-em-all at the end of a PR delete the branch! It's not mean, it's just that trust me! :) If someone is still working on a branch even if the PR is in review, he/she didn't understand the use of branches in the GitFlow model :) But this way, they will learn :joy:
Also: You can mention your entire team by using the @ that I just used above :D