Merge pull request #730 from subspace/contributing-docs

Contributing document

.github/pull_request_template.md

@@ -1,7 +1,2 @@
 ### Code contributor checklist:
-* [ ] I have reviewed my own changes one more time to spot typos, unintended changes, following project conventions, etc.
-* [ ] I have prepared clean readable history of commits before submitting this PR to make reviewer's life easier
-* [ ] I have tested my changes and/or added corresponding test cases (if relevant)
-* [ ] I understand that any changes to this PR going forward will notify multiple developers and will try to minimize them
-* [ ] I have added sufficient description of changes to make review process easier
-* [ ] This PR is ready for review by developers
+* [ ] I have read, understood and followed [contributing guide](../CONTRIBUTING.md)

CONTRIBUTING.md

@@ -0,0 +1,236 @@
+# Contributing
+
+Thanks for considering contributing to Subspace Network!
+
+The best way to start is to join our [Forum](https://forum.subspace.network/) or
+[Discord](https://discord.gg/subspace-network), [official website](https://subspace.network/) has a bunch of learning
+materials too.
+
+GitHub is primarily used by developers to work on new features, improvements and fix bugs. If you're not sure whether
+you have an actual bug that needs to be fixed or you just want to ask a question, forum is the best place for that.
+
+This document describes expectations and recommendations that should make contributions such as issues and pull requests
+on GitHub productive for everyone involved.
+
+## Process optimized for maintainers
+
+Subspace Network is maintained by a small group of passionate individuals with limited time. Number of potential outside
+contributors is much larger than number of maintainers and wider community of users is even bigger than that.
+
+As such, the process of contributing to Subspace Network needs to be optimized in such a way that it doesn't overwhelm
+maintainers with unnecessary notifications, questions that can be answered by wider community and follow practices
+defined in this document below, such that contributions can be processed in the most productive way saving time and
+making it a better experience overall.
+
+## Bugs, improvements and feature requests
+
+These should be submitted in such a way that they are actionable by developers. Again, if you're unsure or just have a
+question - use forum, GitHub issue can be created later if it is necessary.
+
+Always search for similar topics already created on the forum or here on GitHub before creating a new one, but don't
+hijack threads that looks somewhat similar, but are not necessarily the same or have the same root cause.
+
+### Bugs
+
+For bug report to be actionable it should ideally follow the following template:
+
+<details>
+
+### Steps to reproduce
+
+1. On Ubuntu 22.04
+2. Run `subspace-farmer-ubuntu-x86_64-gemini-1b-2022-jun-18 farm` from official release
+3. Observe an error
+
+### Expected result
+
+Farmer starts successfully
+
+### What happens instead
+
+This error
+```
+error: The following required arguments were not provided:
+    --reward-address <REWARD_ADDRESS>
+    --plot-size <PLOT_SIZE>
+
+USAGE:
+    subspace-farmer-ubuntu-x86_64-gemini-1b-2022-jun-18 [OPTIONS] --reward-address <REWARD_ADDRESS> --plot-size <PLOT_SIZE>
+
+For more information try --help
+```
+
+</details>
+
+That is an invalid bug report, of course, but you get the idea of what it should look like.
+
+Try to provide as much useful information as possible, but not too much and without rambling, so that developers can
+quickly diagnose and address it.
+
+### Improvements and feature requests
+
+When there is something that can be improved or a feature is lacking, it is probably a good idea to start with the forum
+and search for similar ideas and if there isn't a topic already, create one to see if this would be a welcome change.
+
+Again, try to discuss with wider audience whether it is a good idea before reaching out to maintainers directly.
+
+When it is necessary to create an issue here, try to describe in a concise way what would the improvement or new feature
+look like, potentially using a similar template as for [reporting bugs](#bugs) describing expected user flow there.
+
+## Code contributions
+
+Code contributions are more involved, but the simplest rule here is to observe what maintainers do, how existing code
+looks like and mimic that, but we provide more specific requirements below.
+
+### Code style
+
+We follow traditional Rust code style with some clarifications and set in `rustfmt.toml`. We also follow all Clippy
+lints (with rare suppressions). As such every commit should successfully pass `cargo fmt` checks and produce zero
+warnings when running `cargo clippy`. This rule includes not just library and application code, but also examples and
+tests.
+
+Every file should end with one and exactly one new line, configure your IDE to insert it automatically. Lines should
+not end with whitespaces, this includes code, comments, documentation (`*.md` files), configs and everything else.
+
+Pay attention to existing code and mimic its structure, for instance we keep dependencies sorted order in `Cargo.toml`
+and if you submit changes where new dependency is added out-of-order you're introducing unnecessary entropy and irritate
+maintainers.
+
+If there is an option between being smart with a tricky piece of code or obvious, prefer obvious. Write idiomatic Rust
+as good as you can, this often allows to avoid needing `.unwrap()`.
+
+Struct definition and its implementation should be in the same file, ideally with nothing else in between.
+
+In blockchain code use of `.unwrap()` is strictly forbidden and must never be used (except test code), use `.expect()`
+instead. `.expect()` must convince reviewer that it will never panic or why panicking might be the preferred outcome
+(rarely, but sometimes happens). `.expect()` correctness should be derivable from local context if it is a standalone
+function or invariants of a data structure for internal method. If it can't be guaranteed then `Result<T, E>` must be
+used instead. There are rare exceptions to this such as block numbers that never exceed `u32` in Subspace.
+
+Indexing into slices and similar data structures with `[]` should be avoided and replaced with `.get()` or similar
+methods with explicit handling of cases where element doesn't exist.
+
+Same goes about mathematical operations, especially in runtime logic, in most cases use explicit checked, wrapping or
+saturating math, but don't use saturating math just to make things compile if business logic doesn't expect overflow to
+ever happen, use checked math with `.expect()` in such cases or return an error.
+
+Unsafe code is to be avoided except in special cases. If you can't make code to compile and decide to use `unsafe` to
+bypass compiler, chances are you're doing it wrong and should ask for help to avoid `unsafe`. When you do use `unsafe`
+for valid reasons, just like with `.expect()`, you need to provide a proof convincing reviewer that it is safe to do so
+and can't be exploited with public API from safe Rust.
+
+If code is incomplete or has known issues, make sure to leave a TODO explaining what needs to be done or what isn't
+handled properly. For big things consider creating an issue.
+
+Prefer longer names of variables, variables with 1-3 characters a typically a bad choice. There are notable exceptions
+like using `id` in entity data structure or `i` for a simple iteration over things.
+
+Otherwise, look at existing code and try to do your best.
+
+### Commits
+
+Commits should tell a logical step by step story of the changes and ideally be individually meaningful. Use squashing
+and rebasing during development, reorder commits if it makes more sense that way, there is great tooling out there for
+these kinds of modifications.
+
+Reviewer should be able to go through commits one by one and understand what is being changed, with non-trivial changes
+it is often very hard to review the final diff, so going through commits helps a lot. For this commits should be
+well-structured and there should be a reasonable number of them. 70 commits for 100 lines or changes is most likely bad,
+same with one commit that changes many thousands of lines of code in various places at once (lock files are a frequent
+and notable exception).
+
+If you work on a branch and after a few commits notice that API introduced earlier doesn't work well or need to revert
+some changes, amend original commit instead of creating a new one much later, since reviewer going through individual
+commits will likely have a comment about something that was already fixed and will simply have to spend more time
+reviewing changes back and forth.
+
+Different kinds of changes should ideally be in different commits if not pull requests entirely. For instance if you
+want to move something and change, move in one commit so reviewer can have a quick look without thinking about it too
+much and apply actual changes in a separate commit. Same with file renaming, if you rename file and change it
+significantly in the same commit it'll make reviewer's life very difficult and likely convince Git that one file was
+deleted and another file added, which breaks tooling for tracking file changes across renames.
+
+It is sometimes the case that some refactoring needs to be done during feature development, try to extract those
+refactoring commits you have just done into a separate branch, submit it for review and rebase your working branch on
+refactoring branch so that the changes are clearly separated and easier to review as an example.
+
+Please push your commits regularly, but not necessarily every commit since it may occupy CI time unnecessarily.
+
+And finally, write commit messages in present tense (as opposed to past) and explain changes that should be done, for
+instance here is a good example:
+> finalize header at archiving depth and prune any forks at that number
+
+Avoid garbage commit names like "fix", "wip", "🤦 x 2", "......", "AHHHHHHHHH" (those are real examples). In most cases
+those commits probably shouldn't exist separately and better squashed with some other commit.
+
+Here is a good article from GitHub on this topic:
+[Write Better Commits, Build Better Projects](https://github.blog/2022-06-30-write-better-commits-build-better-projects/).
+
+### Pull requests
+
+It is important to remember that there are many people subscribed to the repository and any changes to pull request
+will trigger notifications to tens, hundreds, potentially thousands of people. As such try to minimize them or else
+many people, including maintainers, will unsubscribe and response time for issues and pull request changes will increase
+dramatically.
+
+Before even submitting pull request make sure to really do self-review. This will allow you to spot minor issues like
+typos or code you didn't intend to commit in the first place. At this stage use force-pushes to edit original commits to
+make sure you like the changes yourself.
+
+As explained in [commits](#commits) section above, history should be clean and readable, ready for reviewer to go
+through and make sense of changes proposed.
+
+Make sure to test your own code locally and/or in CI before submitting a pull request to avoid numerous updates
+afterwards to fix CI (remember, those changes will trigger extra notifications that could be avoided by following this
+simple rule). Make sure to add new test cases if applicable.
+
+Pull request should be accompanied by reasonable description. If the change is a trivial typo, description can be
+avoided completely, but if changes are non-trivial, make sure to explain what changes do, why, what were the
+alternatives and so on, giving reviewer proper context into changes proposed. If there is a relevant issue or another
+pull request then link it, if there is an issue that is resolved by proposed changes or PR that becomes obsolete, use
+corresponding keywords as described in
+[documentation](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue).
+
+Pull request should be ready for review, avoid drafts that are frequently updated, in most cases you can share WIP
+changes with someone just by sharing a branch diff. Drafts have a narrow and rare use case, use them as last resort,
+otherwise they become a huge source of distraction due to number of notifications they generated. In most cases you
+either want changes to be reviewed and merged, in which case regular pull request should be created, or you don't want
+pull request at all.
+
+Once PR is created, a rule of thumb is to avoid force pushes since it is not always easy to understand what has changed
+in this case and forces maintainers to re-review pull request from scratch. Prefer meaningful commits on top instead.
+One exception to this is fixing typos or trivial renaming, in that case GitHub will show that force push had trivial
+changes, make sure to not rebase on some branch at the same time, it will result in huge diff! It will also potentially
+make old commits non-compilable or will cause tests fail, which hurts potential future debugging. Better merge `main`
+later with a button on GitHub. Another exception is when major refactoring is requested that changes the shape of the PR
+completely, especially if it reduces its size, in such case it is also easier to re-review rather than analyzing diff on
+top of undesirable changes that will pollute the history (opening another PR removes previous context, so generally is
+not recommended, but remains an option).
+
+In case you need to test fixes unknown CI issue for branch that already has PR created for it, create a different test
+branch and push/force-push commits there until you fix it instead of updating PR all the time. If you notice something
+is missing in the PR, convert it to draft temporarily, so it is clear for others that it is incomplete.
+
+If there were changes requested and addressed, make sure to resolve those that are trivially 100% addressed by
+recently pushed changes and leave others to resolve by the person that requested changes. Once you have addressed review
+comments with responses or code changes, make sure to re-request review from that person such that they are aware that
+PR is ready for review again. If there are several unrelated changes requested, it is better to create a few commits
+and push them all at once as opposed to pushing multiple commits one by one or pushing one big commit addressing all
+feedback. For a few simple changes one commit is fine though.
+
+In case there are a few branches that depend on each other, you can submit a PR targeting branch other than `main` and
+once that branch is merged into `main`, target branch in the pull request will be updated automatically, that way you
+can submit a few distinct PRs from a bigger set of changes in a way that is easier for review, though this is only
+applicable to those having access to creating branches in the repository.
+
+When leaving more than one comment, make sure to post them as a review from "File changed" tab, you can do that even if
+you authored pull request. Once done with comments, submit a review all at once, this also minimized amount of
+distraction for maintainers.
+
+Merging of pull requests should be done without squashing in most cases. This makes it possible to track when changes
+happen, leave moving and refactoring of files separated when done in the same PR, helps with bisection of problematic
+changes. Only trivial changes should be squashed or those that are not reviewable for other reasons, though above
+documentation explains how to avoid that.
+
+In general, try to maximize reviewer performance because code is written once by you, but will be read many times by
+maintainers, external auditors and anyone else who might be interested later on.