3 ms·

Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are e

by trgn 7y ago

Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start writing code and designing software in similar ways. These early discussion are generally more jovial. The actual review can focus on details, and, if feedback was gathered and put into practice early, nobody will mind the occasional nitpick about punctuation and naming. These will feel earned, even welcome, the PR being a final touch of polish.

pjm331 7y ago
Out of curiosity how do those conversations happen before the PR? Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”
- trgn 7y ago
  In person, or for a remote job, with screensharing and online chats. In my current job, it generally starts with the implementer reaching out and saying, I want to achieve xyz and think about doing abc. That's how the conversation gets started. Granted, sometimes you need code examples already ready-made to gather meaningful feedback, but that can be personal branch, one-off patch, gist, WIP PR ... feedback can happen over slack, simple chat, comments on the WIP PR ... As long as the final ready-for-review PR doesn't catch the reviewers off guard in terms of overal design/approach.
- underwater 7y ago
  WIP PRs are fine. Essentially the PR is used as a mechanism to clearly communicate an idea, rather than containing code that is intended to be merged into the codebase. There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive. Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
- yoz-y 7y ago
  For smaller changes it can happen over a coffee in the hallway. For large changes there are design reviews. For stuff in between you can schedule a meeting with a few concerned people.

by trgn 7y ago

pjm331 7y ago

Out of curiosity how do those conversations happen before the PR? Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”

trgn 7y ago
In person, or for a remote job, with screensharing and online chats. In my current job, it generally starts with the implementer reaching out and saying, I want to achieve xyz and think about doing abc. That's how the conversation gets started. Granted, sometimes you need code examples already ready-made to gather meaningful feedback, but that can be personal branch, one-off patch, gist, WIP PR ... feedback can happen over slack, simple chat, comments on the WIP PR ... As long as the final ready-for-review PR doesn't catch the reviewers off guard in terms of overal design/approach.
underwater 7y ago
WIP PRs are fine. Essentially the PR is used as a mechanism to clearly communicate an idea, rather than containing code that is intended to be merged into the codebase. There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive. Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
yoz-y 7y ago
For smaller changes it can happen over a coffee in the hallway. For large changes there are design reviews. For stuff in between you can schedule a meeting with a few concerned people.