Page MenuHomePhabricator

Add developer guide to CONTRIBUTING.md
ClosedPublic

Authored by Mengerian on Dec 4 2017, 19:42.

Details

Summary

Explain the development process we want, and how
to work with the project's repository and tools.

Test Plan

Read the guide, make changes based on feedback
from people trying to follow the steps

Diff Detail

Repository
rABC Bitcoin ABC
Branch
abc-developer-guide
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1240
Build 1240: arc lint + arc unit

Event Timeline

There is a README.md in the root of the repository. There is also usually a CONTRIBUTING.md or something similar there. It would be worthwhile to move some of this, or direct people to this, from there. (See: https://github.com/blog/1184-contributing-guidelines)

doc/abc-developer-guide.md
17

after it is accepted?

42

This is very platform dependent - would be good if the doc pointed this out and gave links to the Phabricator user documentation, e.g.

https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/

and the "Installing Arcanist" section in

https://secure.phabricator.com/book/phabricator/article/arcanist/

50

SSH key -> SSH public key

71

There are some prerequisite tools for linting that are needed depending on what files are being modified.

Known to me are clang-format-3.8 and autopep8 .

jasonbcox added inline comments.
doc/abc-developer-guide.md
35

Seems like this section duplicates a lot from docs/developer-notes, including some notes in this outstanding review: https://reviews.bitcoinabc.org/D720 Is this file intended to replace developer-notes?

There is a README.md in the root of the repository. There is also usually a CONTRIBUTING.md or something similar there. It would be worthwhile to move some of this, or direct people to this, from there. (See: https://github.com/blog/1184-contributing-guidelines)

OK, Yeah, I will change the Diff to put all this content in CONTRIBUTING.md

That's a better place for it

doc/abc-developer-guide.md
35

Good point, I didn't notice D720.

We should avoid duplication.

Mengerian retitled this revision from Add developer guide for Bitcoin ABC to Add developer guide to CONTRIBUTING.md.Dec 5 2017, 00:43
Mengerian marked 3 inline comments as done.
Mengerian added inline comments.
doc/abc-developer-guide.md
71

I think they installed automatically for me in Ubuntu.

I will add a comment though, to mention that people may have to install them if it doesn't happen automatically

Add content to CONTRIBUTING.md instead of a separate file

fixes to address freetrader's comments

Apparently, I didn't submit some of my earlier comments, so they're getting submitted now. However, I'm good with this document as-is.

doc/abc-developer-guide.md
16

This will be confusing for a lot of developers. I do not normally work this way, and it's forcing me to rethink how I go about manipulating the code. Although, I think it's for the better. We should discuss in depth what we mean, and how it improves quality. It's sort of a measure-twice cut-once type deal.

18

I want to encourage people to reach out for a 1-on-1 review so things move quickly

78

FWIW, The single quotes are extraneous.

doc/abc-developer-guide.md
35

FYI I just pushed D720, so feel free to rebase and move that content if you think it fits better in CONTRIBUTING.md

Mengerian added inline comments.
doc/abc-developer-guide.md
35

OK, yeah, I think it's better to have this stuff in CONTRIBUTING.md, so I'll move things over as appropriate.

doc/developer-notes.md is full of old Core stuff, a lot of it should probably be gotten rid of, so it's not the best place for useful information right now.

Mengerian marked 2 inline comments as done.

Address @schancel comments
Fix some Markdown formatting
Rebase

Mengerian added inline comments.
doc/abc-developer-guide.md
16

I'm trying got capture how Amaury wants people to work in the project.

I won't land the diff unless he is happy with this part

CONTRIBUTING.md
24 ↗(On Diff #1943)

Sometime, you want to replace one subsystem by another implementation, in which case it is not possible to do things incrementally. In such case, you keep both implementation in the codebase at once for a while (this is more or less what's happening for cashaddr right now).

https://www.gamasutra.com/view/news/128325/Opinion_Parallel_Implementations.php

25 ↗(On Diff #1943)

master without a capital M

27 ↗(On Diff #1943)

Ideally, we'd like to fix what broke rather than revert. But if fixing takes too long => revert. The important point here is not to revert, but to keep master green as much as possible.

CONTRIBUTING.md
114 ↗(On Diff #1943)
git commit --am

does wonders.

doc/abc-developer-guide.md
16

For what it's worth, we have this policy at LinkedIn. Smaller diffs (usually) influence the following:

  • Easier, more digestible chunks that can be reviewed by reviewers in a single sitting. This makes it more likely to get quicker reviews, especially if you require reviews from multiple people.
  • Less likely to run into large merge conflicts.
  • Having one or more test changes/additions per diff encourages code coverage* (*This doesn't apply to all diffs, such as refactors that display the same behavior and are covered by existing tests)
  • Writing code in small chunks can influence how you design your changes from a higher level. Personally, I've found it to be much easier to pivot and make design changes as you get reviews on small changes. For example, if you know you'll be working on a large change, committing skeleton code with /*TODO: <some short description*/ where appropriate can make your design intentions clear without wasting your time on the implementation details. This is especially effective when the design hasn't been agreed on or is not well understood.

Just my two cents.

Mengerian marked 3 inline comments as done.

Make changes based on Deadalnix comments

Mengerian added inline comments.
CONTRIBUTING.md
114 ↗(On Diff #1943)

cool, TIL

This is indeed a better workflow

More detailed descriptions
Fixes and typos

As someone looking to contribute, I'm really glad I stumbled on this PR! I will go through the "Getting set up" steps later today and report back if I run into any issues or see ways in which the documentation would be clearer (of course please don't wait for me to go ahead and merge, I can always make my own diff later if necessary). One thing that seems to be missing, from my perspective as a potential contributor is, what do I work on? I could go and look through the list of tasks on Phabricator, but I'd rather get an idea of what other devs are working on and what they feel is important to be done (and of course other devs can get a feel for my skillset and help point me in a more effective direction). What would be a good way to ask what to work on, is there a chat room or a mailing list or something? And if there is, could we add it to this guide?

As someone looking to contribute, I'm really glad I stumbled on this PR! I will go through the "Getting set up" steps later today and report back if I run into any issues or see ways in which the documentation would be clearer (of course please don't wait for me to go ahead and merge, I can always make my own diff later if necessary). One thing that seems to be missing, from my perspective as a potential contributor is, what do I work on? I could go and look through the list of tasks on Phabricator, but I'd rather get an idea of what other devs are working on and what they feel is important to be done (and of course other devs can get a feel for my skillset and help point me in a more effective direction). What would be a good way to ask what to work on, is there a chat room or a mailing list or something? And if there is, could we add it to this guide?

Probably a good place to start is the Slack channel at https://btcforks.slack.com/

I should also probably mention that, and the list of tasks (https://reviews.bitcoinabc.org/maniphest/) in the guide.

Add "What to work on" section

I think this is pretty good and better than having nothing. Let's land this and we can improve on it over time.

This revision is now accepted and ready to land.Dec 7 2017, 13:21
This revision was automatically updated to reflect the committed changes.