search

CPSSD / robots

CA224 robotics project
4 stars 0 forks source link

CONTRIBUTING.md initial commit #53

Closed DarraghGriffin closed 8 years ago

formal commented 8 years ago

Good document. Two points:

  1. Some text about the use of waffle.io might be useful.
  2. Should a user name be embedded in a branch name.
  3. Are we going to do code reviews when code is merged into master?
  4. I don't think that a PR is a scrum item.
    • Items dragged to the Done column are closed and only items closed in the last week are displayed. Therefore, currently we have a Completed column so that scrum items are not closed and we can see all the items completed in the current sprint. A PR will need to be closed and would have to be moved to the Done column and would most likely disappear before the end of the sprint.
    • Scrum items have a milestone attached after they are selected from the backlog. PRs do not have milestones attached.
    • We could still use In Progress for PRs or we could use the Real Issues column.
DarraghGriffin commented 8 years ago
  1. I have added some text about Waffle use.
  2. I don't think so as multiple users could commit to one branch. I will include this if you think we should.
  3. Yes, this is included in the "Pull Requests" section.
  4. I didn't know that pull requests would appear on Waffle, is there any way we can change this in the settings?
RyanConnell commented 8 years ago

Everything seems ok to me.

JackSmith54 commented 8 years ago

Sounds good to me too

conor-f commented 8 years ago

LGTM

formal commented 8 years ago

Okay

cgmc commented 8 years ago

I have a couple of questions (2 of which I think we discussed in the meeting last Wednesday, but haven't been addressed here yet):

  1. The Google style guide looks fairly dense (I believe @formal brought this up). Did we agree that we are still using it anyway, or was an alternative proposed?
    • If we are staying with Google's guide: if we notice style violations during a code review, perhaps we could ensure that we reference the relevant guideline(s) in our review comments?
  2. Regarding line 32: I think there was discussion (between @formal and @DarraghGriffin) around allowing the PR's creator to merge the code as long as there had been a code review. Are we changing this rule, or leaving it as-is?
  3. About the code reviews themselves: do we think we need a section outlining the process? Or is it sufficiently intuitive for everyone? (A potential issue I'm thinking of is: do we allow a member to make a commit without a substantive description or explanation of what they're modifying and why? "Add section on Waffle" is fine as a description when that's all you're doing, but if you're going to change 20 lines of code which are spread out over a large file, I don't think "fixing a number of issues" would be a sufficiently clear description. Perhaps we should require a Waffle comment outlining the changes before such a commit - or we could limit the number of changed lines per commit.)
DarraghGriffin commented 8 years ago

In response to @cgmc

  1. I think we should use it anyway, there is quite a lot to it but there isn't really another good C++ style guide out there. We don't need most of things in the guide anyway as it deals with a lot of features we may not use.
  2. I think someone else should merge the code as this ensures that there has been a code review.
  3. I don't think we need to outline the process, it isn't very formal. Just need to read the code in the pull request and comment on any code errors or formatting errors. If you don't understand something in the code then you can ask for clarification. If it's not obvious what the submitter is doing then maybe they should add comments explaining the code.
formal commented 8 years ago

I'm not sure, but I think a PR will need to be moved or will automatically move to the Done column, but this is okay as its not a sprint item to be discussed at the review.