From 516a48741f4d7860434f2e007eeb331c537e4c27 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:06:58 -0800 Subject: [PATCH 1/8] docs: update contribution guidelines to add section on commit structure --- docs/code_contribution_guidelines.md | 44 +++++++++++++++++++++++++--- docs/ruby-thing.rb | 12 ++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 docs/ruby-thing.rb diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 1aaad73b..be4f59f8 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -7,8 +7,9 @@ 4.2. [Testing](#Testing)
4.3. [Code Documentation and Commenting](#CodeDocumentation)
4.4. [Model Git Commit Messages](#ModelGitCommitMessages)
-4.5. [Code Spacing](#CodeSpacing)
-4.6. [Protobuf Compilation](#Protobuf)
+4.5. [Ideal Git Commit Structure](#IdealGitCommitStructure)
+4.6. [Code Spacing](#CodeSpacing)
+4.7. [Protobuf Compilation](#Protobuf)
5. [Code Approval Process](#CodeApproval)
5.1. [Code Review](#CodeReview)
5.2. [Rework Code (if needed)](#CodeRework)
@@ -290,9 +291,44 @@ either a '+' or a ','. This prefix seems minor but can be extremely helpful in determining the scope of a commit at a glance, or when bug hunting to find a commit which introduced a bug or regression. + + +#### 4.5. Ideal Git Commit Structure + +Within the project we prefer small, contained commits for a pull request over a +single giant commit that touches several files/packages. Ideal commits build on +their own, in order to facilitate easy usage of tools like `git bisect` to `git +cherry-pick`. It's preferred that commits contain an isolated change in a +single package. In this case, the commit header message should begin with the +prefix of the modified package. For example, if a commit was made to modify the +`lnwallet` package, it should start with `lnwallet: `. + +In the case of changes that only build in tandem with changes made in other +packages, it is permitted for a single commit to be made which contains several +prefixes such as: `lnwallet+htlcswitch`. This prefix structure along with the +requirement for atomic contained commits (when possible) make things like +scanning the set of commits and debugging easier. In the case of changes that +touch several packages, and can only compile with the change across several +packages, a `multi: ` prefix should be used. + +Examples of common patterns w.r.t commit structures within the project: + + * It is common that during the work on a PR, existing bugs are found and + fixed. If they can be fixed in isolation, they should have their own + commit. + * File restructuring like moving a function to another file or changing order + of functions: with a separate commit because it is much easier to review + the real changes that go on top of the restructuring. + * Preparatory refactorings that are functionally equivalent: own commit. + * Project or package wide file renamings should be in their own commit. + * Ideally if a new package/struct/sub-system is added in a PR, there should + be a single commit which adds the new functionality, with follow up + induvidual commits that begin to intergrate the functionality within the + codebase. + -#### 4.5. Code Spacing +#### 4.6. Code Spacing Blocks of code within `lnd` should be segmented into logical stanzas of operation. Such spacing makes the code easier to follow at a skim, and reduces @@ -342,7 +378,7 @@ Functions should _not_ just be laid out as a bare contiguous block of code. -#### 4.5.6. Protobuf Compilation +#### 4.7. Protobuf Compilation The `lnd` project uses `protobuf`, and its extension [`gRPC`](www.grpc.io) in several areas and as the primary RPC interface. In order to ensure uniformity diff --git a/docs/ruby-thing.rb b/docs/ruby-thing.rb new file mode 100644 index 00000000..922201fe --- /dev/null +++ b/docs/ruby-thing.rb @@ -0,0 +1,12 @@ +#!/usr/bin/env ruby + +File.open("INSTALL.md", 'r') do |f| + f.each_line do |line| + forbidden_words = ['Table of contents', 'define', 'pragma'] + next if !line.start_with?("#") || forbidden_words.any? { |w| line =~ /#{w}/ } + + title = line.gsub("#", "").strip + href = title.gsub(" ", "-").downcase + puts " " * (line.count("#")-1) + "* [#{title}](\##{href})" + end +end From 0dc9b35a1a6f2556eda0b14490971e3155a6ce1e Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:09:24 -0800 Subject: [PATCH 2/8] docs: add section in testing pointing towards Makefile docs --- docs/code_contribution_guidelines.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index be4f59f8..445d61e0 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -165,6 +165,10 @@ A quick summary of test practices follows: contained within `lnd`. For example integration tests, see [`lnd_test.go`](https://github.com/lightningnetwork/lnd/blob/master/lnd_test.go#L181). +Throughout the process of contributing to `lnd`, you'll likely also be +extensively using the commands within our `Makefile`. As a result, we recommned +[perusing the make file documentation](https://github.com/lightningnetwork/lnd/blob/master/docs/MAKEFILE.md). + #### 4.3. Code Documentation and Commenting From d1571badafbfc28e579d29465df78c552fd76d9d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:10:04 -0800 Subject: [PATCH 3/8] docs: add section in contribution guidelines for unique lnd code style --- docs/code_contribution_guidelines.md | 91 ++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 445d61e0..90e2bb5e 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -10,6 +10,7 @@ 4.5. [Ideal Git Commit Structure](#IdealGitCommitStructure)
4.6. [Code Spacing](#CodeSpacing)
4.7. [Protobuf Compilation](#Protobuf)
+4.8. [Additional Style Constraints On Top of gofmt](ExtraGoFmtStyle)
5. [Code Approval Process](#CodeApproval)
5.1. [Code Review](#CodeReview)
5.2. [Rework Code (if needed)](#CodeRework)
@@ -380,6 +381,64 @@ Functions should _not_ just be laid out as a bare contiguous block of code. return witness ``` +Additionally, we favor spacing between stanzas within syntax like: switch case +statements and select statements. + +**WRONG** +```go + switch { + case a: + + case b: + + case c: + + case d: + + default: + + } +``` +**RIGHT** +```go + switch { + // Brief comment detailing instances of this case (repeat below). + case a: + + + case b: + + + case c: + + + case d: + + + default: + + } +``` + +If one is forced to wrap lines of function arguments that exceed the 80 +character limit, then a new line should be inserted before the first stanza in +the comment body. +**WRONG** +```go + func foo(a, b, c, + d, e) error { + var a int + } +``` +**RIGHT** +```go + func foo(a, b, c, + d, e) error { + + var a int + } +``` + #### 4.7. Protobuf Compilation @@ -415,6 +474,38 @@ Notice how the `json_name` field option corresponds with the name of the field itself, and uses a `snake_case` style of name formatting. All added or modified `proto` fields should adhere to the format above. + + +#### 4.8. Additional Style Constraints On Top of `gofmt` + +Before a PR is submitted, the proposer should ensure that the file passes the +set of linting scripts run by `make lint`. These include `gofmt`. In addition +to `gofmt` we've opted to enforce the following style guidelines. + + * ALL columns (on a best effort basis) should be wrapped to 80 line columns. + Editors should be set to treat a tab as 4 spaces. + * When wrapping a line that contains a function call as the unwrapped line + exceeds the column limit, the close paren should be placed on its own + line. Additionally, all arguments should begin in a new line after the + open paren. + + **WRONG** + ```go + value, err := bar(a, + a, b, c) + ``` + + **RIGHT** + ```go + value, err := bar( + a, a, b, c, + ) + ``` + +Note that the above guidelines don't apply to log messages. For log messages, +committers should attempt to minimize the of number lines utilized, while still +adhering to the 80-character column limit. + ### 5. Code Approval Process From ce39910f964f63a67f4ac365c8ccb6130b81bf10 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:10:34 -0800 Subject: [PATCH 4/8] docs: add section in contribution guidelines describing fixup commits --- docs/code_contribution_guidelines.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 90e2bb5e..9f18edea 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -560,6 +560,13 @@ master. In certain cases the code reviewer(s) or interested committers may help you rework the code, but generally you will simply be given feedback for you to make the necessary changes. +During the process of responding to review comments, we prefer that changes be +made with [fixup commits](https://robots.thoughtbot.com/autosquashing-git-commits). +The reason for this is two fold: it makes it easier for the reviewer to see +what changes have been made between versions (since Github doesn't easily show +prior versions like Critique) and it makes it easier on the PR author as they +can set it to auto squash the fix up commits on rebase. + This process will continue until the code is finally accepted. From 0d5eef4f304cf900f9c42ccfbdab4adc8e6b83f1 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:11:24 -0800 Subject: [PATCH 5/8] docs: in contributor guidelines, use `make check` not `go test` --- docs/code_contribution_guidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 9f18edea..2458442a 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -602,7 +602,7 @@ Rejoice as you will now be listed as a [contributor](https://github.com/lightnin - [  ] For code and documentation: lines are wrapped at 80 characters (the tab character should be counted as 8 characters, not 4, as some IDEs do per default) -- [  ] Running `go test` does not fail any tests +- [  ] Running `make check` does not fail any tests - [  ] Running `go vet` does not report any issues - [  ] Running [golint](https://github.com/golang/lint) does not report any **new** issues that did not already exist From a76affae91337bab2d1682cebd5a09e785b31fd5 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:11:46 -0800 Subject: [PATCH 6/8] docs: replace golint with `make lint` in contributor guidelines --- docs/code_contribution_guidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 2458442a..6f7ec55e 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -604,8 +604,8 @@ Rejoice as you will now be listed as a [contributor](https://github.com/lightnin per default) - [  ] Running `make check` does not fail any tests - [  ] Running `go vet` does not report any issues -- [  ] Running [golint](https://github.com/golang/lint) does not - report any **new** issues that did not already exist +- [  ] Running `make lint` does not report any **new** issues that + did not already exist From 49da4a60aaec89f790575f6d856ba8cce142d661 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 12 Dec 2018 19:12:05 -0800 Subject: [PATCH 7/8] docs: update contribution guidelines with new merge commit policy --- docs/code_contribution_guidelines.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 6f7ec55e..73f3861b 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -573,11 +573,14 @@ This process will continue until the code is finally accepted. #### 5.3. Acceptance -Once your code is accepted, it will be integrated with the master branch. -Typically it will be rebased and fast-forward merged to master as we prefer to -keep a clean commit history over a tangled weave of merge commits. However, -regardless of the specific merge method used, the code will be integrated with -the master branch and the pull request will be closed. +Once your code is accepted, it will be integrated with the master branch. After +2+ (sometimes 1) LGTM's (approvals) are given on a PR, it's eligible to land in +master. At this final phase, it may be necessary to rebase the PR in order to +resolve any conflicts and also squash fix up commits. Ideally, the set of +[commits by new contributors are PGP signed](https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work), +although this isn't a strong requirement (but we prefer it!). In order to keep +these signatures intact, we prefer using merge commits. PR proposers can use +`git rebase --signoff` to sign and rebase at the same time as a final step. Rejoice as you will now be listed as a [contributor](https://github.com/lightningnetwork/lnd/graphs/contributors)! From af306e236079c4970d673954692165144848b37d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 19 Dec 2018 17:02:25 -0800 Subject: [PATCH 8/8] docs: expand contributor checklist w/ commit structure --- .github/pull_request_template.md | 6 ++++++ docs/code_contribution_guidelines.md | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 2ca61829..90f47ff8 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,5 +1,7 @@ #### Pull Request Checklist +- [ ] If this is your first time contributing, we recommend you read the [Code + Contribution Guidelines](https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md) - [ ] All changes are Go version 1.11 compliant - [ ] The code being submitted is commented according to the [Code Documentation and Commenting](#CodeDocumentation) section @@ -17,3 +19,7 @@ - [ ] Running `go vet` does not report any issues - [ ] Running `make lint` does not report any **new** issues that did not already exist +- [ ] All commits build properly and pass tests. Only in exceptional + cases it can be justifiable to violate this condition. In that case, the + reason should be stated in the commit message. +- [ ] Commits have a logical structure ([see section 4.5, of the Code Contribution Guidelines]) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 73f3861b..1d06b5a7 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -609,6 +609,10 @@ Rejoice as you will now be listed as a [contributor](https://github.com/lightnin - [  ] Running `go vet` does not report any issues - [  ] Running `make lint` does not report any **new** issues that did not already exist +- [  ] All commits build properly and pass tests. Only in exceptional + cases it can be justifiable to violate this condition. In that case, the + reason should be stated in the commit message. +- [  ] Commits have a logical structure (see section 4.5).