Add an initial guide on review culture

[teamdandelion]

Lately we've been discussing how cultivating review culture
across SourceCred will help us stay aligned as we grow our community. In
particular, I think we should start producing shared "docs artifacts"
that we've reviewed and we stand behind as a community. By "docs
artifacts" I mean both things like user guides, and also community
touchstones.

So, to kick things off, I wrote this guide on review culture, which I
hope will be one of the first docs artifacts. Please review it and let
me know what you think. :)

[teamdandelion]

Some notes to reviewers:

• If you're someone who has extensive experience contributing to open-source projects on GitHub, this all may seem obvious to you. However, bear in mind that for some SC contributors, helping out on SC is their first exposure to open-source culture and to GitHub. At some point (maybe quite soon) we'll want to invest in documenting how to use GitHub for non-coders.
• I wonder if using "change" as the orienting word in this vocabulary is somewhat confusing... since reviewers often "request changes" to the "change"

[s-ben]

Good stuff. Review culture definitely adds value, and I think this is great for code and official documentation. I just have some concerns (expressed in comments) about GitHub becoming a bottleneck for certain types of docs (e.g. user guides). And some concerns about it being used as a general purpose governance mechanism (e.g. statement of values). Here we could have some issues around centralization.

[s-ben]

Mixed feelings about all documentation going through this process. For technical documentation (e.g. SC user docs, developer docs, etc.), it makes sense. This is basically what all OSS projects do. No need to reinvent the wheel. You do need to have some control over critical documents, for security purposes sometimes (especially if money/crypto is involved), and also because some people will want "official" docs they have a higher level of confidence in. At the same time, the trend in the last few years is that much of the documentation is unofficial, crowdsourced from users in the form of StackOverflow posts, personal blog posts, forum posts, etc. Most user guides I see for larger products/projects are just individual users documenting their process to install/use something. Often these guides are collectively better than what the company can produce. We want to make sure those people get in the graph and get rewarded. Making people use GitHub might be a low barrier for OSS projects (where most use GitHub already). But once you go outside of that into other domains, that could be a significant barrier.

For SourceCred, I think "official" documentation should live in GitHub (/docs is a good idea). But I think we should recognize that a lot of the crowdsourced documentation/guides may only surface on Discourse (or other platforms we create plugins for). Perhaps that is valued less. But it should not be zero.

[Beanow]

Agreeing with your points here. But to be clear, do you think the current wording suggests all documentation must follow this review process? (I didn't read it that way, but if it does read like that, it's probably worth rephrasing.)

[s-ben]

I suppose this wording:

By "docs artifacts" I mean both things like user guides, and also community touchstones.

Seemed fairly broad. Though upon rereading, I think it's just the 'user guides' part that seems like a potential bottleneck. Maybe we could just say 'official user guides'.

[vsoch]

My reading was that it was about documentation in the context of GitHub, but you're right it could be inferred to be from elsewhere. If GitHub isn't thought of as a central place to host version controlled docs (with sources coming in from everywhere via plugins, etc.) then we probably need to clarify this.

[teamdandelion]

Updated the phrasing:

A change can take many forms, for example:

• Adding or changing official SourceCred documentation (in sourcecred/docs)
• Adding or changing the SourceCred codebase

[s-ben]

Maintainers already have a lot of power here. If we start flowing money according to scores, that power will increase. Wary of abuse, but also bottlenecks. For instance, let's say SourceCred gets big, and is installed on all sorts of different OSse, hardware, etc. And configured in all sorts of different ways. That means we'll start seeing a lot of different guides pop up (e.g. how to install SourceCred on Mac Mojave, configured for my problem). A maintainer will probably not have the bandwidth or ability to review all these. If a contributor adds an HTML link to an outside guide to the docs, does that flow cred to the guide's author? The contributor that submits the PR?

For "core documentation", which is widely applicable, I think a maintainer makes sense.

[Beanow]

I think they're fair points. Though I think it's less about maintainers and more about when we should or shouldn't use a review process.

For example, contributions that don't have a review process (wiki's, forums, ...) there are also fewer appointed maintainers and when they're there, it's much more hands-off. But when it does have a review process, there's likely an involved maintainer as well.

Reviews cause centralization, so we should be mindful where we apply it.

[s-ben]

While this makes sense for code changes, changes to official docs, I think relying on GitHub as a governance mechanism (as is done in Bitcoin, Ethereum, etc.) leads to problems of centralization when using it for more broad governance proposal. For instance, if someone proposes a change to a document artifact adding a new principle to a values statement, does the maintainer get a voto due to their monopoly on merge access? Something like this seems better decided with cred-weighted voting or other mechanism.

[Beanow]

Definitely share this view. While GitHub may have an open culture, from a technical perspective it uses authorities (the maintainers). To protect you from those authorities, you have this button
^{One-click to be president of your own corner of the internet!}

And this works very well for things that are inexpensive to copy. Like documentation and code.

But when something won't be easily copied:

• Like the people in a community
• Or the domain name it has
• Or the funding it received

That Fork button gives you no protection at all. Nice people might try to reduce the 'cost of forking'. But it's not when you agree with nice people that you need protections 😄.

So using only GitHub as your governance model, is the equivalent of giving a sitting president the power to veto the outcome of an election... There are better ways to do it.

---

That said. Reviewing, maintaining and governing are very different things so I think it's unrelated to "Review culture". Let's make a document in this repo to share some information about how the SourceCred community does governance.

[s-ben]

Let's make a document in this repo to share some information about how the SourceCred community does governance.

This is a good idea. Though the process is in enough flux right now I'm not sure it's worth the effort yet. Once we settle on some more formal governance mechanisms (e.g. get cred weighted voting going), and those are fairly stable, it makes sense to document them well. For now, linking to the TBD post on Discourse post as we've been doing seems to suffice. Though as we build out user guides for newcomers, we should be on the lookout for opportunities to convey all of this stuff better.

[teamdandelion]

Thanks for the review, @s-ben.

Let me give some context on where I'm coming from with this change. I think our discourse-centric approach to guides, documentation, and collaborating has revealed some pitfalls. Most notably, it's too easy for folks to charge off in different directions, in a way that is very well-intentioned but not sufficiently well-coordinated. The result is more heat than light, and activity without the production of durable artifacts that we can all agree on and stand behind.

You raise a really good point that getting merged into this repository shouldn't be a bottleneck for creating or receiving cred for documentation. Guides that are posted just to Discourse, or just to third party websites and blogs, are still very valuable, and their authors should still receive cred. In practice, this means there should be mechanisms for flowing cred (e.g. boosting, people liking things and thanking them) that do not discriminate between "official" docs and unofficial ones.

However, I think as a community, we do need a way to canonically bless some docs as being official. This ensures there's a core basis by which new people can acquaint with the project--both at a technical and values level--and know that they're leaving with definite knowledge that the project stands behind. That's what I hope this repository will do.

You raise good points about centralization, and the potential that maintainers will wind up with too much power. I appreciate your suggestion that we use cred-weighted voting as a way to resolve contentious changes; futarchy boosting is also an interesting proposal.

Over the long run, it's really important that we decentralize this decision making. However, part of the challenge of creating SourceCred is that it's both an attempt to define an idealistic system, but also an attempt to ship a working system an imperfect world, with imperfect tools. That means tradeoffs. One of those tradeoffs is that at the moment is that we're accepting a greater degree of centralization in the short term than we want in the long term, so that we can develop the level of cohesion we need for this project to actually achieve success.

[s-ben]

Makes sense. I agree we do need some centralization right now and some canonical docs. This article on progressive decentralization I posted on Discourse makes good arguments around that. I mainly just raise issues around centralization and barriers to entry so we have them in mind and can avoid unnecessary "centralization traps" or technical debt.

The main goal I keep coming back to (here and elsewhere) is that things of value should never be zero. They can be lower value. E.g. a post on Discourse that's not quite a "user guide", but still useful to people with a specific problem, should not get as much cred as an official guide. But it should have a node somewhere in the graph. Getting that node will introduce its author to SC, hopefully engage them further, and potentially get them more cred down the line should that node become retroactively valuable. For example, if I'm creating some official docs in this repo, and I use a code example and some text from said Discourse post, I want to be able to reference that and flow them some cred.

Have some experience creating the kind of canonical docs we want, and have some ideas and best practices to share. But perhaps I should open a new Issue for that?

[teamdandelion]

Have some experience creating the kind of canonical docs we want, and have some ideas and best practices to share. But perhaps I should open a new Issue for that?

Let's avoid using GitHub issues for discussions; I think Discourse is a much better tool for that. However, if you have ideas for best practices on producing canonical docs, I would love to have a canonical doc explaining this, e.g. a CONTRIBUTIONS.md.

Maybe a good place to start would be to discuss your ideas in a Discourse thread, and then if we agree that we want to canonicalize some of those suggestions as "how we do canonical docs", then you can issue a PR. What do you think?

[Beanow]

Cheers to this document 🎉 and a great way to start off this repository imo 😄

These are some suggestions reading through it individually (before looking into s-ben's comments).

[wchargin]

Above you say that “every single change to the system is reviewed by at
least one other person before becoming official”. Should it be noted
somewhere that at least one reviewer must be a maintainer? I think that
this is a helpful distinction, because one common point of confusion
from those unfamiliar with free software is, “if anyone can change it,
how doesn’t it just get broken from poor contributions, much less overt
vandalism?” Indicating that at least one reviewer must be explicitly
trusted rather than “anyone in the community” addresses this.

May also want to mention something about this in “Approval and merging”
below.

[teamdandelion]

I actually don't think it's a requirement that every approved change was approved by at least one maintainer. Rather, I think every change needs to accumulate "sufficient review" for its level of complexity and potential controversy.

For example, suppose there's a change which fixes a typo in the README. Because the complexity and potential for controversy is minimal, a review by any reasonably-trusted member of the community is sufficient. On the other hand, a massively invasive cross-cutting change may need approval from two maintainers before being approved (in practice this might look like the first maintainer commenting with "this makes sense to me, but because it's so invasive, i'll let (other_maintainer) give canonical approval".

im open to an improvement to the guide that elaborates on this... weak preference to merge the guide as is. (as a general principle, i don't want merging docs to block on every potential improvements--that's what future pull requests are for.)

[Beanow]

+1, to add a section about it. But also suggest a new PR for this.

(In my experience this point varies wildly between communities. So this document seems like the right place to touch on it.)

[vsoch]

I think (at the end of the day) someone with write access has to be the one to merge the PR, so a sense the final say always comes down to someone that is technically a maintainer. But a maintainer being pinged to merge a PR that has accumulated sufficient review without reviewing him/herself is different than a maintainer doing the actual review. But if I put myself in those shoes, even if we had a rule about accumulated sufficient review, if I were a maintained pinged to merge it, I'd still want to take a look.

If a tree falls in the woods does it make a sound... if a maintainer browses to a pull request... something something :)

[wchargin]

Suggest adding a section on “nit-and-approve” as an encouraged practice,
calling out the trust considerations. (Or, I can draft such an addition
and submit my own PR, if you’d prefer?)

I have a couple of other best practices for proposers and reviewers in
my mental model, and will try to think about which ones are worth
surfacing in a document like this.

[teamdandelion]

Would love to have a section on nit-and-approve. Please draft the addition as a separate PR.

[wchargin]

Sure: will do. (edit: #2)

[wchargin]

nit: s/Its/It's/

[teamdandelion]

Rephrased in such a way that removed the typo.

[teamdandelion]

I've updated the document based on review feedback. I'm going to leave this PR open for another day or two so that others in the project have a chance to review, and then merge it.

[Beanow]

Thanks btw for making that a second commit. This view helps a lot:
b5ce2ce?short_path=7e47cbf#diff-7e47cbf37b19ba9259e500ff42eb9f18

[wchargin]

Sure: will do. (edit: #2)

[s-ben]

However, if you have ideas for best practices on producing canonical docs, I would love to have a canonical doc explaining this, e.g. a CONTRIBUTIONS.md.
Maybe a good place to start would be to discuss your ideas in a Discourse thread, and then if we agree that we want to canonicalize some of those suggestions as "how we do canonical docs", then you can issue a PR. What do you think?

@decentralion, created a Discourse thread with ideas on how we could move forward with docs.
https://discourse.sourcecred.io/t/creating-documentation-on-github-review-culture/582

Replied to a couple of @Beanow's comments, but generally in agreeance with everything 👍

[vsoch]

This is a hugely annoying detail (and something I don't tend to do for the repos I maintain) but I've seen it in various communities so I want to mention. Should there be standards around how to write the markdown? For example, I've worked with folks that like to have one sentence per line, because it makes editing / commenting / seeing changes easier on GitHub.

[teamdandelion]

I would like to have standards around markdown, and I think they are out-of-scope for a guide on review culture in general. They would, however, be very relevant as contributing guidelines for this repository (sourcecred/docs) in particular. I made an issue on this repo to discuss: #3

[vsoch]

I would add some context around merging, since it's a GitHub term. Maybe like:

at least one other person before becoming official, or "merging," meaning that requested changes are integrated into the main codebase.

This has me thinking that we need an entire "Welcome to GitHub, here are some useful terms" section, but maybe a link (later in the document when it's appropriate) would do the trick - those that are scratching their heads would click, others would continue reading.

[teamdandelion]

Yeah, since we're using GH as source-of-truth for docs, and we want less-technically-experienced contributors to be able to help write docs, we need some guides for them.

[vsoch]

Probably the number of reviewers required for merge would vary based on the repository? Later we should figure out the standard for deciding this. Intuitively, small trivial things == one review, if it takes more than 5 minutes, 2 reviewers, and more robust changes (code or docs) should have 3-4.

[wchargin]

I agree that it depends on project and context, but in my experience it
has been far more effective to bias toward fewer reviewers than more.
Pulling in two reviewers for a non-trivial change is a huge productivity
hit, both because the extra reviewer takes the time to review the code
and because the presence of more people encourages bikeshedding and
nitpicking. In fact, all projects that I’ve been on that started out
with a many-reviewer culture eventually adopted a guideline of “prefer
one reviewer; if you request multiple, explicitly call out what you want
from each person and why one reviewer doesn’t suffice”, or similar.

[vsoch]

If you've experienced this multiple times, I'm all for fewer reviewers - this feels a bit like (what are they called, anti-patterns?) because intuitively you'd think more is better, but... (insert metaphor with cooks and soup broth). I like the idea of erring toward fewer, and leaving it up to the requester (and possibly the first reviewer) to decide if more are appropriate.

I wasn't familiar with the term bikeshedding - here is a helpful link for others that might not be.

[vsoch]

My reading was that it was about documentation in the context of GitHub, but you're right it could be inferred to be from elsewhere. If GitHub isn't thought of as a central place to host version controlled docs (with sources coming in from everywhere via plugins, etc.) then we probably need to clarify this.

[vsoch]

Since we have various platforms for discussion, it might be useful here to suggest a standard way to alert people on those platforms that "Hey this PR needs some love!"

[BrianLitwin]

This is a really good idea ☝️ . New contributors will probably have less confidence communicating with folks, and err on the side of being less assertive in communicating the state of their requests. It's easy to feel naggy when you're requesting somebodies time/attention.

[vsoch]

To some extent we can also automate this with actions, given that platforms involved have APIs / programmatic access.

[Beanow]

Like the idea, let's continue at #4

[vsoch]

I guess we don't want to consider the reality that some changes don't get accepted, and get a wont-fix?

[teamdandelion]

Good point. Added a line mentioning this case.

[vsoch]

And for this reason, it's suggested to favor more, smaller pull requests over a single, larger one. (or something like that).

[teamdandelion]

Yep. We may want a "best practices for contributors" section that we can mention:

• ask the community for feedback early in the process (e.g. "is this change a good idea"?)
• make small changes
• do self-review

Want to submit a followon PR? (In the spirit of #2)

[Beanow]

Tracking suggestion here #7

[vsoch]

I would add something along the lines of "In these cases, maintainers should openly communicate how their thinking or the situation has changed from the initial opening of the pull request, and make an effort to both thank the contributor, and look for opportunities to still involve him or her with future development."

The general idea is that you wouldn't want to say "OH sorry we don't need / want this anymore" but something more like "Our thinking has changed and we think this [other thing] might be a better strategy to pursue, would you be interested in helping with well scoped task]?

[BrianLitwin]

This ☝️ is a really good idea. Concrete examples are really great IMO, especially to those unfamiliar with the process.

A bit of a tangent, but: It can be really nerve-wracking to submit a pull request; one thing that is not at all nerve-wracking about submitting a PR to SourceCred is formatting your PR in terms of commit structure and message, because instructions and rationale are laid out clearly and comprehensively in the Contributing Guide and there's no grey area. We could aspire to something similar in terms of etiquette; providing concrete examples about things to do in non-ideal circumstances feels like a way to remove grey area.

[vsoch]

It's probably implied, but pull request templates can help with a nice checklist of TODOs when opening the pull request, and specific groups to mention if the requester needs help with a particular issue. For example, I've used GitHub since 2011 but I still have cases when I'm not sure about a particular bullet point in a pull request template. The pull request template should have a catch all for this, because we just can't predict all situations / questions that might have a contributor hesitating and not sure about something. For example, if the requester is in this situation, he/she might be encouraged to open a draft pull request pinging some particular team that can give support before it's ready for review.

[teamdandelion]

Sounds good. Following on from this comment above, maybe we should have a "best practices for maintainers" section too. I'd rather see that in a followon PR so that this one can merge. :)

[Beanow]

Tracking suggestion here #8

[BrianLitwin]

This might be too GitHub-centric of a point, but: Is "requesting a review" the correct terminology? Versus "Submitting your request for changes" i.e. submitting a pull request? It might not be obvious to a new contributor exactly when their work comes to the attention of the community, e.g. they might not think that anybody will notice and start engaging their PR until after they request a reviewer. Whereas it's generally accepted to start reviewing a PR once it's been opened.

[teamdandelion]

IMO opening the pull request is a form of requesting a review. Still trying to keep this guide relatively platform agnostic so not going to change things here (at least not in this PR).

[BrianLitwin]

This is a really good idea ☝️ . New contributors will probably have less confidence communicating with folks, and err on the side of being less assertive in communicating the state of their requests. It's easy to feel naggy when you're requesting somebodies time/attention.

[BrianLitwin]

It might be worth giving some context/expectation about how iterative the review process is. A new contributor might think that getting 10 comments on their PR, for three iterations in a row, signifies they've submitted a bad PR or the reviewer is being overly hard on them. It's not uncommon to see people to "give up" on their PRs after a few rounds of reviews. Whereas, in the optimistic case, that sort of back-and-forth can be really valuable.

[Beanow]

Generally agree with the sentiment. Iterating because of feedback, when constructive, is a win for collaboration 👍, but can be off-putting. Would you like to follow up?

[BrianLitwin]

This ☝️ is a really good idea. Concrete examples are really great IMO, especially to those unfamiliar with the process.

A bit of a tangent, but: It can be really nerve-wracking to submit a pull request; one thing that is not at all nerve-wracking about submitting a PR to SourceCred is formatting your PR in terms of commit structure and message, because instructions and rationale are laid out clearly and comprehensively in the Contributing Guide and there's no grey area. We could aspire to something similar in terms of etiquette; providing concrete examples about things to do in non-ideal circumstances feels like a way to remove grey area.

[teamdandelion]

I've updated the PR with another commit addressing the review feedback from the last few days.

I'd like to bias towards merging docs that are substantively agreed-upon, and not blocking merging on adding every additional improvement we can think of-- that's what followon PRs are for. :) Since this has already gotten two approvals and no objections, I think it's sufficiently reviewed from a substance perspective. Would someone please review the latest commit to ensure it doesn't add any new issues, and then I'll merge on that final approval. And I'll be happy to see more PRs which add more features or improvements to the doc (e.g. #2).

[wchargin]

nit: ”Adding to or changing the SourceCred codebase”?

[teamdandelion]

Done, thank you.

[Beanow]

Unable to make an approval after the merge.

But 👍 I'm happy with the way my comments were handled.
Overall structure also still looks good to me.

---

Also would like to thank everyone joining in. Lots of great points were raised :]
Hoping to preserve some of those, I've given this PR another read to try and track some of the stuff that was suggested and actionable.

There's still comments there that I may have missed, or I didn't track because they need some more discussion before tracking as something we should do.