Revise CONTRIBUTING #4382
Revise CONTRIBUTING #4382
Conversation
- Wrap text at 80 columns. - Match capitalization of GitHub usernames. - Prescribe more rules for pull requests. - Link more reference documentation.
thejohnfreeman
requested review from
intelliot,
seelabs,
mDuo13 and
RichardAH
CONTRIBUTING.md
Outdated
| - `master`: The latest stable release. | ||
| - `gh-pages`: The documentation for this project, built by Doxygen. | ||
|
|
||
| All of your commits must be signed, |
There was a problem hiding this comment.
Maybe leave a note that "sign-off" is not "signing", since there could be confusion around this (some projects require sign-offs for their CLAs for example).
There was a problem hiding this comment.
Might also be worth having this checked by a bot in addition to stating it here.
There was a problem hiding this comment.
I don't think GitHub's built-in protections can enforce this, and I'm not aware of a third-party bot that does it, but it is somewhat enforced by the fact that PRs cannot be squashed and merged unless all commits are signed.
There was a problem hiding this comment.
GitHub's protections can enforce that the tip of any branch is signed. I don't think it's necessary that all commits are signed as long as the tip is. My understanding is that signing a commit is implicitly verifying all the commits leading up to it (sort of how validating a ledger in rippled implicitly validates all the previous ledgers). It may be worth rephrasing this to something like "All of your commits should be signed. The tip of pushed branches must be signed."
There was a problem hiding this comment.
GitHub can enforce that the tip of our branch is signed, not that the tip or any commit of a PR branch is signed. This part of the document is talking about the commits made by contributors, the commits that appear in their PR.
When it comes time to merge a PR using squash-and-merge, GitHub will make the commit and sign it with GitHub's key, but only if all of the commits in the PR are signed by the author. That is why we must require that they sign all of their commits.
There was a problem hiding this comment.
I'll add this explanation to the document.
|
|
||
| ## Formatting | ||
| All code must conform to `clang-format` version 10, unless the result would be unreasonably difficult to read or maintain. | ||
| To change your code to conform use `clang-format -i <your changed files>`. |
There was a problem hiding this comment.
This might be worth keeping in, it should be easier to do the right thing than documenting only how to ignore the formatter. ;-)
There was a problem hiding this comment.
I think that documenting how to use clang-format is outside the scope of this document, but I will add this example back in.
| This preserves the ability for reviewers to filter changes since their last | ||
| review. | ||
|
|
||
| A pull request must obtain **approvals from at least two reviewers** before it |
There was a problem hiding this comment.
"reviewer" in that context means "maintainer" - not "random person on Github clicking on reviewed", right?
There was a problem hiding this comment.
No, because we have reviewers who are not maintainers, e.g. myself. The next sentence is there to state that it must be two qualified reviewers (maybe I'll add that adjective), where "qualified" is in the opinion of the maintainer who merges the PR.
| * [Nixer89](https://github.com/nixer89) (XRP Ledger Foundation) | ||
| * [manojsdoshi](https://github.com/manojsdoshi) (Ripple) | ||
| * [n3tc4t](https://github.com/n3tc4t) (XRPL Labs) | ||
| * [Nik Bougalis](https://github.com/nbougalis) |
There was a problem hiding this comment.
@nbougalis - confirm the way you prefer your name? (if any preference)
CONTRIBUTING.md
Outdated
| choose the next available standard number, and | ||
| open a discussion with an appropriate title to propose your draft standard. | ||
|
|
||
| When you submit a PR for this change, please link the corresponding XLS in the |
There was a problem hiding this comment.
As this file is touched anyway, I believe it is relevant to be more explicit in the purpose and use of XLS:
When you submit a PR for this change, please link the corresponding XLS draft in the description. As XLS drafts are considered work in progress and open for discussion, PRs should not be submitted without due time for discussions, questions, suggestions and additions to the XLS draft. It is the responsibility of the XLS author to update the draft to match the final implementation when the PR is merged.
There was a problem hiding this comment.
I added this after some editing.
|
will merge this today; if anyone has any issues with it, please open a new issue or PR with your desired changes |
* upstream/develop: Revise CONTRIBUTING (XRPLF#4382)
* upstream/develop: Revise CONTRIBUTING (XRPLF#4382)
* upstream/develop: Revise CONTRIBUTING (XRPLF#4382)
* upstream/develop: Revise CONTRIBUTING (XRPLF#4382)
* upstream/develop: Revise CONTRIBUTING (XRPLF#4382)
…ctionality * upstream/develop: Update documented pathfinding configuration defaults: (XRPLF#4409) Update dependency: grpc (XRPLF#4407) Introduce min/max observers for Number Optimize uint128_t division by 10 within Number.cpp Replace Number division algorithm Include rounding mode in XRPAmount to STAmount conversion. Remove undefined behavior * Taking the negative of a signed negative is UB, but taking the negative of an unsigned is not. Silence warnings Introduce rounding modes for Number: Use Number for IOUAmount and STAmount arithmetic Add tests Add implicit conversion from STAmount to Number Add clip Add conversions between Number, XRPAmount and int64_t AMM Add Number class and associated algorithms Revise CONTRIBUTING (XRPLF#4382)
…tpage * upstream/develop: (37 commits) Update documented pathfinding configuration defaults: (XRPLF#4409) Update dependency: grpc (XRPLF#4407) Introduce min/max observers for Number Optimize uint128_t division by 10 within Number.cpp Replace Number division algorithm Include rounding mode in XRPAmount to STAmount conversion. Remove undefined behavior * Taking the negative of a signed negative is UB, but taking the negative of an unsigned is not. Silence warnings Introduce rounding modes for Number: Use Number for IOUAmount and STAmount arithmetic Add tests Add implicit conversion from STAmount to Number Add clip Add conversions between Number, XRPAmount and int64_t AMM Add Number class and associated algorithms Revise CONTRIBUTING (XRPLF#4382) `XRPFees`: Fee setting and handling improvements (XRPLF#4247) Update BUILD.md (XRPLF#4383) Make NodeToShardRPC a manual test (XRPLF#4379) Update build instructions (XRPLF#4376) ...
- Wrap text at 80 columns. - Match capitalization of GitHub usernames. - Prescribe more rules for pull requests. - Link more reference documentation.
These are changes I think belong in CONTRIBUTING. Let's have a discussion. There are some policy changes in here (two reviewers for all PRs) and some long-practiced-but-newly-stated polices added. I appreciate critique from a technical writing perspective as well.