jhanley634
commented
1 month ago 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 testis 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:
- Is it correct?
- 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
audiodude
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.