chore: Add contributing guidelines #874
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Contributing | ||
|
There was a problem hiding this comment. I would like a note here that every PR should do exactly one thing. Not three things and not half of a thing. There was a problem hiding this comment. One thing is subjective. This solves a single goal of making it easier to write code with fewer comments in code review. There was a problem hiding this comment. Nope. One thing is not subjective. Maybe we can define it specifically if it's not clear
There was a problem hiding this comment. I think this document fails a test I just invented: if someone follows all the included or referenced guidelines, will their code sail through review? Other than domain-specific stuff, e.g. how to interact with the EC2 API, of course. There was a problem hiding this comment. Yes, their code will sail through review given functionality makes sense. This document isn't intended to provide exhaustive coverage of contribution but provides general guidelines to make it frictionless. |
||
|
|
||
| ## Go Style | ||
|
|
||
| Contributions must adhere to [Effective Go](https://go.dev/doc/effective_go). Linting rules should | ||
| be preferred over documenting styles (run ours with `make lint`); humans are error prone! | ||
|
There was a problem hiding this comment. BUT if you can't figure out how to make it a linting rule, make it documentation rather than doing nothing. There was a problem hiding this comment. I feel like that's expressed here, but I'm fine to change this wording. |
||
|
|
||
| Read [Go's Code Review Comments Wiki](https://github.com/golang/go/wiki/CodeReviewComments) to find | ||
| common comments made during reviews of Go code. | ||
|
There was a problem hiding this comment. I don't think this is sufficiently helpful direction. |
||
|
|
||
| ### No Unused Packages | ||
|
There was a problem hiding this comment. Will this lead to a lot of "big bang" PRs? Edit: by "big bang" I mean a PR that gets created against a changeset when the author considers the unit of work "100% done". This can lead to large changesets that can take a while to review. There was a problem hiding this comment. It's certainly possible, but if it makes reviews simpler it may be overall less taxing. An example is when we implement OAuth: we could scaffold in one PR, and implement GitHub in another, but then we don't have the context on how it works in a real world implementation. If it becomes unreasonable, I'm happy to loosen up this language. There was a problem hiding this comment. We can also adjust this to There was a problem hiding this comment. Can tests count as "checking against implementation" here? There was a problem hiding this comment. I don't think so, but I should probably make that perspective more explicit. I don't believe we need fully implemented usage of a package to merge, but I think it's helpful context for producing a more cohesive contribution. @johnstcn do you agree with the core principle? I agree with your thoughts that this promotes chunky PRs (which is bad), so if you have any thoughts to modify the phrasing I would happily accept! There was a problem hiding this comment. I agree with the core principle, yes. |
||
|
|
||
| Coders write packages that are used during implementation. It's difficult to validate whether an | ||
| abstraction is valid until it's checked against an implementation. This results in a larger | ||
| changeset but provides reviewers with an educated perspective on the contribution. | ||
|
|
||
| ## Review | ||
greyscaled marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| > Taken from [Go's review philosophy](https://go.dev/doc/contribute#reviews). | ||
|
|
||
|
|
||
| Coders value thorough reviews. Think of each review comment like a ticket: you are expected to | ||
| somehow "close" it by acting on it, either by implementing the suggestion or convincing the reviewer | ||
| otherwise. | ||
|
|
||
| After you update the change, go through the review comments and make sure to reply to every one. You | ||
| can click the "Done" button to reply indicating that you've implemented the reviewer's suggestion; | ||
| otherwise, click on "Reply" and explain why you have not, or what you have done instead. | ||
|
|
||
| It is perfectly normal for changes to go through several round of reviews, with one or more | ||
| reviewers making new comments every time and then waiting for an updated change before reviewing | ||
| again. All contributors, including experienced maintainers, are subject to the same review cycle; | ||
| this process is not meant to be applied selectively or discourage anyone from contribution. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is a great start to our Contributing guide!
What are your thoughts on adding sections for the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add these in another PR. I'm primarily focusing on code style and review in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay in that case I don't think it addresses #646 so we should keep that open and I'll address there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! It's a lil part of it, but certainly doesn't entirely help someone contribute.