-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CONTRIBUTING.md ahead of move of repo to XRPLF #4214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and I personally don't want to bikeshed too much on this; even if there are improvements that could be made, they could be done in a separate PR, but a CONTRIBUTING.md
is needed and has been missing for a long time.
Two brief comments (food for thought, really): consider whether some of the docs under docs/
should be linked here.
I know that @wojake took a swipe at this with PR #3955, and that spurred me to also tackle this as well. Not suggesting either of those are better (mine certainly isn't; it's overly full of emojis! 🤮), but I decided to link those previous efforts here for inspiration.
👍 from me, as is.
Looks way more simple and informative, LGTM. Thank you 👍 |
I'm following up here but not because I think we need to refine this further; as I said during my review, this looks good to me, especially as a first step. But I |
It's definitely intended to be improved / a living document. I think the main focus at the moment is getting the repo into the foundation ASAP, and further policy refinements can be made after |
Please consider adding a I've had a hard time finding the right people to peer review my material in the past, I don't know how hard and lengthy of a process it is to peer review code which configures a decentralized payment network, so I leave this decision to Richard and the others. Bitcoin Core's CONTRIBUTING.md copy looks good to me as a starting point, hopefully we'll tweak it as we progress. |
It's hard to know who the right person to review code is. Again, looking at Bitcoin, they have a file that lists reviewers with expertise in the various parts of the code. It would be good to think about adopting this too. Generally, it's hard to know how long a review takes. It's a process, and it's not always easy or clean and it's rarely pleasant. Even @JoelKatz once complained that he's having a hard time getting his code to pass code review. And that is not necessarily a bad thing, as long the review process is objective.
While that part of the contribution guidelines addresses something different, in reading through your comment I did have a thought that I wanted to raise. @RichardAH, I wholeheartedly agree with your comment that "the main focus" should be on "getting the repo into the foundation ASAP, and further policy refinements can be made after." However, I think that there's a small change we should consider adding now rather than later: I'd change the section about maintainers to include a process for adding new and removing existing maintainers:
|
998bcaa
to
8055252
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor suggestions; looks good overall
cf3edd3
to
e7c4fe4
Compare
Recommend renaming commit: Create |
e7c4fe4
to
b0c5af7
Compare
b0c5af7
to
d48105b
Compare
High Level Overview of Change
Add a
CONTRIBUTING.md
ahead of moving the repository to the XRP Ledger Foundation.If you are listed as a maintainer and do not wish to be listed please comment.
Suggestions / changes welcome.