Create text file to document contribution guidelines

[skyfenton]

As we keep deciding on how tests should be constructed, naming schemes, formatting tools, docstrings, etc. we should have a markdown text file (a "CONTRIBUTING.md") to define our guidelines for writing code, submitting PRs, etc. that we can refer newcomers to and use to remind ourselves.

This way we also have a list of conventions to double check for in PR reviews to maintain consistency and confidence a PR has checked all the boxes.

[jhanley634]

I can share things that I have seen are effective at "herding cats" when working with teams having between two and two dozen engineers of diverse skill levels.

• tests: $ make test is Green, and we have decent coverage. When adding a new feature function, expect that test code will exercise at least one line of that function. Or queue up a subsequent PR to add a test.
• naming: PEP 8. It's easy to do, and it's universal within the python community. Linters and reviewers can flag issues.
• formatting: black, then verify a clean $ ruff check. One can get ruff to write fixes, but I find that makes it harder to communicate an idea to newbies than a simple "black + ruff", "write + read", would be. Even simpler: $ make lint, and have that group imports with isort. I tend to assume that modern python code shall have type annotations -- no reason to omit them.
• docstrings: This is the only topic that is "hard".

We conduct a code review during the PR process to assess two things:

1. Is it correct?
2. Is it maintainable?

The easy-to-run $ make lint test gives an objective answer to the first. I offer this example.

The author likely believes that anyone in the team could fix a bug or add a feature to the code they just wrote. But only another teammate could give an answer of, "yes! I think I grok this new code and could maintain it." That's why we do reviews, to helpfully reveal blind spots in the author's thinking. What is obvious to the author won't be universally obvious, so we give other folks an opportunity to chime in, before they're stuck with supporting that code.

We desire that functions should compose, so each author takes care to spell out a new function's contract. Sometimes the pre- and post- conditions are clear from the signature. If not, we need at least a one-sentence docstring. A reviewer should be able to articulate what the contract is. Imagine there is no unit test. By examining the function's signature and contract (but not its source code), a reviewer should be able to write meaningful unit tests. If not, it is appropriate to push back, to suggest improvements to the PR prior to merging down to main.

[skyfenton]

Those all sound solid to me, I imagine this document would be 70-80% standard python practices with just a little bit of our own preferences for communication and workflow.

[audiodude]

@skyfenton would you like to take a crack at it? There's probably templates you can use to get started.

[skyfenton]

Sure, I'll draft something up. I'll def want to review it next week with everyone to make sure there aren't any important gaps.

[audiodude]

Awesome, sg

[audiodude]

Let's document:

• Adding descriptive commit messages for individual commits
• Merging main instead of rebasing
• Not force pushing active PRs
• Linking issues from PRs

And much much more!

[jhanley634]

Possibly https://github.com/noisebridge/MediaBridge/blob/main/doc/review-procedure.md closes out these concerns? Or possibly we wish a companion document in that directory?

And also I'm willing to believe that in recent months perhaps "John did X" but "we were hoping for X + Y", which might motivate creating a document that makes suggestions about Y. That is, I welcome with open arms any critiques of "John made this SHA1 commit", and "the commit would have been better if we followed policy Y instead". I think I make mistakes all the time, and that we can always learn from them and strive to do better. In any event, I would like to see some resolution to Issue 34 during the month of March, something constructive that comes from it. Sooo, setting a timer for 31st March on this.

Merging main instead of rebasing

Excellent! Since October, the project config has changed. It now specifies that new PR submissions shall be "squashed", which seems a good way to make $ git annotate *.py output reflect the high level goals of an Issue, rather than the day-at-a-time goals of "get this test to run without crashing!".

[audiodude]

I think the review-procedure doc is a good starting point to base CONTRIBUTING.md on. Like I said, the reason to use a CONTRIBUTING.md at the top level of the repo is two-fold:

1. It's a standard place where contributors will know to look for it
2. It's a priveleged location for GitHub. Putting it there allows GitHub to find it and surface it to potential contributors.

As far as mergining instead of rebasing, I was actually refering to a specific set of issues that came up early in the project, where folks were rebasing main onto their PR branches, which led to lots of badly resolved conflicts and confusion with having to resolve the same conflicts multiple times (in the case of a new rebase).