Create contributors.md #3
Conversation
|
Some thoughts: How should we handle merging after approval if the user pushes more commits? In the best case, this is a user doing a rebase before merging. In the worst case, this is a user trying to sneak some code in there. I frequently do the former to rebase onto master after approval. In either case, at least functionally, the code is still passing regressions. Default behavior in GitHub is to have PR approvals be sticky. I.e., once a PR (which is tied to a branch) is approved, that branch is forever approved. It's not doing any intelligent checks to see if the effective diff of the approved version matches the current branch. Alternatively, you can tell GitHub to require re-approval if the branch changes. This is a headache for reviewers and a huge pain considering we're so light on Chisel/FIRRTL reviewers... I can see benefits and drawbacks to both approaches, but generally think that we should keep things as it is. With things in their current state it means that people who can merge code should be highly trusted. What does it mean to be tagged on a review? If somebody is tagged on a review, must they review or is that only a reviewer suggestion? E.g., Jack is assigned to review some PR, but I jump in and approve it. From GitHub's perspective, that code is approved and I can merge it. Can I do that? In what situations would that be okay or not okay? Who merges the code? Rocket Chip has very low latency from PR approval to merge. Sometimes things linger in Chisel/FIRRTL for awhile. I think we should adopt this for Chisel/FIRRTL/Big6. Basically, the PR approver makes a call. If they approve the code and there's nobody else that should review, then it's approved immediately following approval. If somebody else should review it, then the approver requests a review from them and indicates on the issue that merging is pending their approval. Maybe Mergify has some approaches for automating these types of interlocks. RFC template? I was looking over the SIP template and this looks great. Unfortunately, GitHub doesn't seem to have an API for differentiating between RFC's and issues. One approach would be to make the RFC the PR template and keep the issue template as is. This is a bit overkill and fails to allow the user to submit an RFC without code. One approach could be to move RFCs out of the mainline repos into something else. Either a common |
|
TODO:
|
This is definitely a WIP
Pull Request Process included some stuff that contributors without write access can't actually do so I'm including it here instead of in the initial commit:
Pull Request Process (additional stuff)
Please mark your pull request with one (or more) of the following Review Status tags:
New Thoughts - add these in PR template, and auto scrape to label them:
When you submit a pull request, please assign a reviewer. You can pick either a suggested reviewer,
the firrtl-reviewer team, or any other reviewer.
If the reviewer you select isn’t familiar enough with the part of the codebase you are modifying,
they may add additional reviewers (so don’t worry about that).