Contributing to YunoJuno
This is the opening doc that we share with new team members, outlining what
we expect of them in terms of coding standards and git etiquette. It's not
a lot more than 'be a good citizen', but thought some others may find it
interesting.
First off, welcome aboard. This isn't a public project [ clearly this isn't
true, as I've just published it, but it was never supposed to be ;-) ],
so if you're reading this it means you're on the team. Good to have you with us.
Second, before you commit your first code changes, or push your first
commit, there are a few things we'd like to go through. We have a high
percentage of freelancers on the team (deliberately - we are a community for
freelancers, built by freelancers) , and so the "code is read more often than
it is written" philosophy really strikes a chord. As such we put a lot of
emphasis on both the readability of the code, the associated documentation,
and critically, the git commit logs.
This doc is about those aspects.
History
Whilst this doc is all about best practice, we are a bootstrapped startup,
and we can't claim to be perfect. There are some inconsistencies within the
core YJ codebase that date back to the very early days - and some code is not
where you might expect it to be. Some isn't even very good. That's life.
This document is intended to prevent any further lapses.
Python Source Code
All source code should be PEP8'd. This includes:
- line lengths
- spaces instead of tabs (4 please)
- spacing between classes and functions
- docstrings
The Google Style Guide is a good resource for style tips. From their "Parting Words":
BE CONSISTENT.
If you're editing code, take a few minutes to look at the code around you and
determine its style. If they use spaces around all their arithmetic operators,
you should too. If their comments have little boxes of hash marks around them,
make your comments have little boxes of hash marks around them too.The point of having style guidelines is to have a common vocabulary of coding
so people can concentrate on what you're saying rather than on how you're
saying it. We present global style rules here so people know the vocabulary,
but local style is also important. If code you add to a file looks drastically
different from the existing code around it, it throws readers out of their
rhythm when they go to read it. Avoid this.
Comments
There are plenty of people on Hacker News who would claim that comments are
unnecessary, and that the code should be comment enough. We don't intend to
employ them, and don't subscribe to that theory. Comments are useful, and
should be sprinkled liberally throughout. Be verbose, descriptive, hell, crack
a joke or two. Just don't leave us trying to work out what on earth you were
doing.
Documentation
Larger areas of the application have their own documentation in a separate repo,
which you've already cloned, as it's the same repo that this is in. Read them,
they're full of useful information and background.
This more formal documentation, covering large areas of the application, their
role and how they fit within the overall design of the solution, are kept in a
separate repository so that they can be updated without triggering any automated
processes.
There is no hard-and-fast rule regarding what should be formally documented -
it generally comes out of a conversation that starts with "how does X work",
and ends with "why don't you write that down". As with comments - be verbose - the more you write the more you'll get it clear in your own mind, and the more
people get to edit your docs, the more the more the knowledge is shared,
refined and improved.
Python Tests
TDD isn't a religion for us, but tests, whilst not compulsory, are very, very,
important for all of the core business processes. We don't have full-time QA,
we develop fast, deploy often, and ensuring we haven't broken anything is
absolutely critical.
Every app should have a good set of Django tests that encompass models & views. Everything that has side-effects must be covered. Everything that changes FSM state must be tested. HTTP response codes should be validated (200, 302),
including all common exception scenarios (403, 404, 410, etc.).
Basically, we won't hold you to a coverage number, but you should be confident
that another developer can be confident that if all the tests run ok, then the site won't break.
Test responsibly.
Git Commits
Our CTO likes reading, our lead dev was a journalist - so we take a lot of
pride in our commit notes, and alongside your code, this is the most important
part of the job. The following rules apply:
- Commit early and often.
- Commit atomically - small, complete commits that serve a single purpose.
- Rebase -i locally if necessary before pushing to central repo.
- Always run the full test suite before pushing.
- Channel your inner Linus when writing commit notes:
- ... a single headline, < 72 chars,
- ... followed by a blank line,
- ... followed by a verbose description of the changes. Be creative.
- Headline should describe what the commit does, not what you did.
The end goal is to have a git history that can be read by someone and easily
understood. Atomicity is important because without it it becomes impossible to
cherry-pick commits between branches. Don't be afraid to commit a single char
change if it's appropriate.
Here is a sample from our existing git history (headlines only):
Enforce one contract per brief / freelancer combination rule.
Fix for duplicate contract creation bug (Elin).
Improvements in timesheet admin.
Bump django-errordite version to 0.6.
Rename fields from 'limited company X' to just 'company X'
Management command for generating weekly invoices.
Tests for invoice state changes (django_fsm) and minor refactorings.
And here is a complete commit (headline and description):
Adds context processor for analytics keys, and adds mixpanel tracking.
The Google and Mixpanel analytics keys should now be set in settings.py
(and read from env vars). They are added to all template contexts so that they can be injected into templates - and can support different keys for different environments (to prevent dev testing from polluting live logs).
In the course of a normal working day you should be pushing to the central repo
at least once, preferably more often. Each push should contain multiple atomic
and logical commits. Pushing a single commit at the end of a day with a title
of "Today's work" is a 'P45** error'. Don't do it.
(** for those outside the UK, the P45 is the official tax document that you
receive when you leave a job. It's also a metaphor for sudden termination of
employment - it's analagous to a 'pink slip' in the US.)
Git Workflow
We use git-flow
as our core workflow. You can read more about it, but the essentials are:
- Two core branches - master and dev
- Regular development should be committed to dev branch
- No commits directly into master branch
- Feature branches taken off dev branch, merged back into dev
- Hotfix branches taken off master branch, merged back into dev and master
Git Deployments
Deployments are git pushes of the master branch.
The intention is that the master branch is always stable, and deployable. The
process of deploying involves:
- merging dev branch into master
- running test suite against master
- deploying master to UAT environment
- validating migrations against live data backup (anonymised)
- smoke testing application
- deploying to live
All of the above is automated and run through Fabric, and, if all of the
previous notes are taken into account, is be a pain-free process.
Each deployment is labelled on master. A changelog
is generated from the non-merge commits between labelled deployments.
Making Freelance Work