chore: Add contributing guidelines #874
Conversation
It's helpful for us Coders to align on a common set of style guidelines. While I'd prefer to automate this, documentation should get us a lot of the way there! Please review these thoroughly, as PRs will be checked against it after merge.
c929bab to
201f4c2
Compare
Codecov Report
@@ Coverage Diff @@
## main #874 +/- ##
==========================================
- Coverage 65.93% 65.43% -0.50%
==========================================
Files 204 212 +8
Lines 13252 13382 +130
Branches 86 103 +17
==========================================
+ Hits 8738 8757 +19
- Misses 3622 3725 +103
- Partials 892 900 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Suggestion(optional): We sometimes use conventional comments. It's not required, but can help. Could be a good link out (in the review section) in case people come across it and are curious
https://conventionalcomments.org/
Not sure if you feel it fits in (borders too much info). Thought I'd leave this as an optional suggestion.
|
We can add standardized commits as a listing rule in the future - I'm extremely game for that! I'm hesitant to add in this PR to avoid scope bloat. |
There was a problem hiding this comment.
Suggestion: Can we limit the line-length to 80? We used to have our prettier and eslint stuff at the root of the repo for purposes such as this. I don't feel compelled to move it now and/or inflate this effort, so another way to go about this is for me to ask: are you ok if we prettier-ify these docs once we configure it for use across the repo?
|
I'm certainly game for that... long markdown lines are terrible to edit! We'll have to move the |
| [Go's Code Review Comments Wiki](https://github.com/golang/go/wiki/CodeReviewComments) | ||
| to find common comments made during reviews of Go code. | ||
|
|
||
| ### 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 Prefer Unused Packages if you feel strongly this will lead to poor decomposition.
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.
I have an idea for a second principle that I want to try out personally before forcing onto everyone else.
I'd prefer to not enforce an 80-character line length limit. 100 would probably be OK though. |
| @@ -0,0 +1,37 @@ | |||
| # Contributing | |||
There was a problem hiding this comment.
I feel like this is a great start to our Contributing guide!
What are your thoughts on adding sections for the following:
- Requirements: what do I need on my machine to contribute?
- PRs: see code-server
- Development Workflow: walk me through the steps
There was a problem hiding this comment.
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.
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.
Agreed! It's a lil part of it, but certainly doesn't entirely help someone contribute.
|
|
|
@johnstcn I'm down to bump to 100. I shall make the change! I didn't realize the kernel doesn't recognize 80 chars anymore, that's cool too. |
|
|
||
| ## Review | ||
|
|
||
| > Taken from [Go's review philosophy](https://go.dev/doc/contribute#reviews). |
docs/CONTRIBUTING.md
Outdated
| 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. |
There was a problem hiding this comment.
I think the title misleads the point here - we prefer simple and repetative over pre-mature abstractions. I think we really just want to say "repetition is fine and abstractions can come after the pattern fully emerges". Unused packages are just one form of this.
There was a problem hiding this comment.
I feel this is a clear example of pre-mature abstraction. I intentionally avoided abstract language to avoid abstract interpretation. I'm happy to lead with that quote prior to examples of common misuse, and we follow it up with these clear examples.
There was a problem hiding this comment.
I'm not sure how to expand on this in a way that isn't super abstract. I think as we run into cases where this is problematic we should refine it.
There was a problem hiding this comment.
I want to be cautious about adding abstract cases without examples. I picked the unused package since it's a common pattern for decomposition, with a clear message why it's wrong. I'm in favor of rolling with this for now, and if we still see problems of duplicative nature, we create another rule.
|
@presleyp @vapurrmaid @deansheather any last remarks before the merge? |
| be preferred over documenting styles (run ours with `make lint`); humans are error prone! | ||
|
|
||
| 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.
| ## 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.
| @@ -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
- add a feature
- refactor
- fix a bug
- reformat
- add documentation (except when attached to a new feature)
| @@ -0,0 +1,32 @@ | |||
| # Contributing | |||
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.
It's helpful for us Coders to align on a common set of style
guidelines. While I'd prefer to automate this, documentation
should get us a lot of the way there!
Please review these thoroughly, as PRs will be checked against it
after merge.