docs(i): Update Contributing Guidelines #3522
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3522 +/- ##
===========================================
- Coverage 78.51% 78.50% -0.01%
===========================================
Files 397 397
Lines 37599 37599
===========================================
- Hits 29519 29515 -4
- Misses 6390 6394 +4
Partials 1690 1690
Flags with carried forward coverage won't be shown. Click here to find out more. see 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think you have tried to be too comprehensive here, and I fear that the extra words will discourage would-be-contributors and hide the truely key bits of information within this document.
Consider getting Ashutosh's thoughts on this, as IMO it is an important document.
|
|
||
| You are encouraged to join the [Source Network Discord](https://discord.gg/w7jYQVJ) to discuss ideas, ask questions, and find inspiration for future developments. | ||
| It is recommended that before you begin, familiarize yourself with the following: |
There was a problem hiding this comment.
suggestion: Most of these are only relevant to fairly small sections of the codebase, and listing them here may discourage people as it presents Defra as more complicated than it is.
I strongly suggest the removal of everything apart from Project documentation, Git, GitHub and Go.
I also suggest the removal of GitHub and Git, as the use of only very small subset of features is required to successfully contribute is required, and suggesting the read the entire documentation is going to be a waste of time and off-putting.
I suggest the removal of Go, because it is kind of obvious, and, if they are going to contribute to .go files, it is the first thing they are going to google if they need to.
| pkgsite | ||
| # open http://localhost:8080/github.com/sourcenetwork/defradb | ||
| ``` | ||
| - Refer to [go.dev/doc/comment](https://go.dev/doc/comment) for guidelines on writing Go doc comments. |
There was a problem hiding this comment.
suggestion: I suggest removing the go-docs stuff from here, as we pay very little attention to it, and we may waste the limited attention and enthusiasm of people reading this document by diverting them to the go-doc docs and our poorly maintained ones.
|
|
||
| - [Go](https://go.dev/doc/install) | ||
| - Cargo/rustc, typically installed via [rustup](https://www.rust-lang.org/tools/install) | ||
| - [SourceHub](https://github.com/sourcenetwork/sourcehub), installed via `make install` |
There was a problem hiding this comment.
todo: IMO these 3 lines are the most important in the entire document and you appear to have removed them. Please put them back, preferably near the start of the document.
| - Cargo/rustc, typically installed via [rustup](https://www.rust-lang.org/tools/install) | ||
| - [SourceHub](https://github.com/sourcenetwork/sourcehub), installed via `make install` | ||
| ### Sign the CLA | ||
| Read and accept the Contributor License Agreement (for first-time contributors). |
There was a problem hiding this comment.
question: What license?
| 8. Pushing your feature branch with the commit changes, on to your fork. | ||
| 9. [Opening a pull request](#opening-pull-request) from your branch on your fork, targeting the `develop` branch of the main repository. | ||
|
|
||
| ### Limited Use of Branch Flow |
There was a problem hiding this comment.
suggestion: This document is for new contributors, I recommend removing this section - it is a waste of reading-time for them and makes the project appear more complex/daunting than it needs to appear.
| - Refer to [go.dev/doc/comment](https://go.dev/doc/comment) for guidelines on writing Go doc comments. | ||
| ### Asking for review | ||
| - Request a review from the *database-team*. | ||
| - Discuss and adapt the pull request as needed by following the commenting etiquette defined below. |
There was a problem hiding this comment.
suggestion: Neither of these are required, I suggest their removal in order to keep this document focused.
|
|
||
| ## Testing | ||
| ### Title Format |
There was a problem hiding this comment.
suggestion: IMO this is section is either in the wrong place, or duplicated, and should be moved or removed.
I prefer removal, as it is also duplicating the documentation that should be present in the workflow that actually performs this check, and as such will be very obvious to contributors when it fails, and I don't want us to spend time maintaining duplicate documentation.
| 3. Creating a feature branch on the clone of your forked repository. | ||
| 4. Making changes on the feature branch. | ||
| 5. Writing tests for the changed behavior, if applicable. | ||
| 6. Ensuring that all the `make` target checks are passing: |
There was a problem hiding this comment.
nitpick: The CI does (6), I would avoid spending space/attention here on it.
|
|
||
| 3. Make changes on the feature branch. | ||
| 4. Write tests for the changed behavior, if applicable. | ||
| 5. Ensure that all the `make` target checks pass: |
There was a problem hiding this comment.
suggestion: This is a duplicate of an early section, and optional. I suggest removal of (5).
| - `make mocks` | ||
| - `make lint` | ||
| - `make tidy` | ||
| - `make test` |
There was a problem hiding this comment.
suggestion: Figuring out how to compile/run the tests can be a pain in some repos, and needs to be done very early on in development - I suggest adding a How to compile and run the tests section, preferably early on in this document.
I think you are right in some areas there is "more" details then necessary. Mostly due to not knowing where to put some of the things (most of these belong on a newly on boarding members wiki page that are internal specific). Some new PRs that are coming in, it would be nice to pin point one location for them to go for all their "PR related conventions, i.e. title syntax, etc."
Good idea, @ashutosh-src |
Relevant issue(s)
Resolves #3353
Description