CONTRIBUTING.md

[pinkforest]

Description

Canonical CONTRIBUTING.md

Motivation

Usually people flock to readme / CONTRIBUTING.md to look for advice around on how to contribute / repo conventions etc.

e.g. #2803 (comment)

Current Implementation

Thomas mentioned the file was supposed to be there 👀 ? 🤷‍♀️

EDIT: Probably worth linking here https://github.com/ipfs/community where CONTRIBUTING.md is - if it's the correct place ?

p.s. that CONTRIBUTING.md also says push -f

Are you planning to do it yourself in a pull request?

Maybe

[thomaseizinger]

Thanks for opening the issue!

EDIT: Probably worth linking here ipfs/community where CONTRIBUTING.md is - if it's the correct place ?

p.s. that CONTRIBUTING.md also says push -f

Those guidelines seem to be mostly influenced from the golang side (they also credit Docker at the bottom). In practice, we use different ones in here.

@mxinden I thought we had some guidelines somewhere that explain our merge-squashing, no force pushes, the PR title convention etc?

[mxinden]

I am only aware of https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md and #2780.

I am in favor of adding additional information for newcomers. The only thing I would like to watch out for is not overwhelming newcomers with a large text mandatory to read before contributing. In other words, I would rather explain these guidelines on each pull request than loose a lot of contributors on the way to their first pull request.

[pinkforest]

Explaining these things separately is just setting people to fail - contributors don't like failing.

It's a bit like google way of not blaming the people but the tools instead when / if anything goes wrong -

Either implicitly - even if nobody meant so - or explicitly - it's friction -

People can get pretty touchy about doing something they were never told about or where the tooling just set them up to fail :)

It's better to have clear and minimal CONTRIBUTING.md if you want people to behave in a certain way.

I can usually skim things pretty fast and CONTRIBUTING.md is pretty standard thing to have -

I probably spend 30 sec on any CONTRIBUTING.md and I've rarely seen CONTRIBUTING.md that overwhelms me yet -

Only exception is when it is written by people who don't have a programming / FLOSS experience and one has to decipher it.

CONTRIBUTING.md should explain how to set up build environment, tests and things like that e.g. what someone has to run before raising a PR - It should not dictate how to follow some arbitrary manual rules that are plain cosmetic.

[thomaseizinger]

I am only aware of https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md and #2780.

I am in favor of adding additional information for newcomers. The only thing I would like to watch out for is not overwhelming newcomers with a large text mandatory to read before contributing.

I agree. The ipfs-community one is way too long IMO: https://github.com/ipfs/community/blob/master/CONTRIBUTING.md

I'd like to see a CONTRIBUTING.md that informs users about:

• Our use of squash merging
• Not using force-push on PRs
• Setting a PR title that starts with the the files/modules that are being modified (i.e. core/muxing)

Additionally we can mention that contributors should:

• Run cargo custom-clippy locally
• Run cargo fmt locally

[pinkforest]

Requiring people not to force push is tbh asking too much -

People work in different ways and any deviation out of ordinary is just friction.

In the end it's their branch and PR and you can choose whether to accept or not.

I would not set any arbitrary rules on that and deal with that in the tooling e.g. using bors-ng.

Most people usually miss --all-features and --all-targets

Also what I would like to see in docs is how to test this whole thing.. or separate parts of things

The repo is pretty void re; information how to test / validate changes besides people running cargo test if one is lucky

[thomaseizinger]

Well, ultimately, we obviously can't force (pun intended) users to not force push but I see it as a "these are our conventions, please follow them if you can".

Because we squash merge, individual commits don't have to be pretty / build / pass clippy / formatting so the use of force pushing just makes reviews harder.

Regarding testing, there isn't really anything to do besides running cargo test --all-features. If CI passes, a PR is typically good to go. I agree that this should be mentioned though :)

[thomaseizinger]

Closing in favor of a summarized discussion in: #3743