Make sure you have set up the prerequisities.
You can get an environment for developing the entire project using nix-shell in the root directory.
This includes a variety of useful tools:
-
purs -
spago -
purs-tidy -
purescript-language-server -
… and more
Have a look in shell.nix to see what’s there.
We rely heavily on this approach to ensure that everyone has consistent versions of the tools that we use. Please do make use of this, since problems due to mismatched versions of tools are particularly annoying to fix!
|
Note
|
You may want to consider using lorri as a convenient alternative to running nix-shell directly.
|
The shell comes with some tools for fixing various simple problems that the CI will complain about. Specifically:
-
fix-purs-tidywill re-format all the Purescript sources correctly.
If you’re not using nix-shell all the time and you want to run one of these, you can use nix-shell --run.
The nix-shell environment has purs and spago installed.
From here you can build the project packages directly with spago build.
The nix-shell environment has a purescript-language-server executable.
Follow the instructions for your editor to configure it:
|
Note
|
you must run your editor from the nix shell, and not from, say, an application launcher like spotlight or dmenu for your editor to find it in its PATH. |
To build in CI, we use spago2nix. If you install a new library or update one,
you need to run spago2nix to update spago-packages.nix accordingly.
|
Important
|
These files needs to be regenerated if you change any dependencies in spago.dhall or packages.dhall files. But the CI will tell you if you’ve failed to do so. |
We pin versions of some git repositories that are used by Nix, for example nixpkgs.
We use Nix flakes to manage these dependencies and build the package. To manage the dependencies, see the Nix flake inputs documentation and the Nix flake lock command.
Specifically, you will probabl want to say nix flake lock --update-input <input-name>.
Do not use nix flake update, as that will update all the inputs, which we typically don’t want to do.
We are a relatively large team working on sometimes quite abstruse problems. As such, it’s important that future people who work on the project know how things work, and just as importantly, why. These future people may even be yourself - we forget things very quickly!
When writing, try to put yourself in the position of someone coming to this code for the first time. What do they need to do to understand it and do their job? Write it down!
Code review is a good lens for this: if you have to explain something to a reviewer, then it is probably not clear in the code and should have a note.
This applies both to the code itself (structure, naming, etc.) and also to comments. How to write useful comments is a large topic which we don’t attempt to cover here, but Antirez is good. If in doubt: write more!
One special kind of comment is worth drawing attention to. We adopt a convention (stolen from GHC) of writing fairly substantial notes in our code with a particular structure. These correspond to what Antirez calls "design comments", with some conventions about cross-referencing them.
The structure is:
-
The Note should be in a multiline comment (i.e.
{- -}) -
The first line of the Note should be
Note [Name of note] -
Refer to a Note from where it is relevant with a comment saying
See Note [Name of note]
For example:
{- Note [How to write a note]
A note should look a bit like this.
Go wild, write lots of stuff!
Here's a small diagram:
A ----> B >> C
And of course, you should see Note [Another note].
-}
Notes are a great place to put substantial discussion that you need to refer to from multiple places. For example, if you used an encoding trick to fit more data into an output format, you could write a Note describing the trick (and justifying its usage!), and then refer to it from the encoder and the decoder.
We use purs-tidy for code formatting.
CI checks that running this is a no-op, so if you don’t apply it them your PR will not go green.
To avoid annoyance, set up your editor to run them automatically.
The nix-shell environment provides a purs-tidy binary of the correct version.
You can run purs-tidy over your tree using the fix-purs-tidy script providedby the nix-shell environment.
The CI builds PureScript code using psa with --strict, so will fail if thereare any compiler warnings.
So fix your own warnings!
Please make informative commit messages! It makes it much easier to work out why things are the way they are when you’re debugging things later.
A commit message is communication, so as usual, put yourself in the position of the reader: what does a reviewer, or someone reading the commit message later need to do their job? Write it down! It is even better to include this information in the code itself, but sometimes it doesn’t belong there (e.g. ticket info).
Also, include any relevant meta-information, such as ticket numbers. If a commit completely addresses a ticket, you can put that in the headline if you want, but it’s fine to just put it in the body.
There is plenty to say on this topic, but broadly the guidelines in this post are good.
A pull request is a change to the codebase, but it is also an artifact which goes through a change acceptance process. There are a bunch of things which we can do to make this process smooth which may have nothing to do with the code itself.
The key bottleneck in getting a PR merged is code review. Code review is great (see below), but it can slow you down if you don’t take the time to make it easy.
The amount of time it’s worth spending doing this is probably much more than you think.
Having a sensible and comprehensible set of changes makes your reviewer’s life much easier.
-
Keep commits to a single logical change where possible. The reviewer will be happier, and you’ll be happier if you ever have to revert it. If you can’t do this (say because you have a huge mess), best to just have one commit with everything in it.
-
Keep your PRs to a single topic. Including unrelated changes makes things harder for your reviewers, slowing them down, and makes it harder to integrate new changes.
-
If you’re working on something that’s likely to conflict with someone else, talk to them. It’s not a race.
A pull request is communication, so as usual, put yourself in the position of the reader: what does your audience (the reviewer) need to know to do their job? This information is easy for you to access, but hard for them to figure out, so write it down!
However, better to put information in the code or commit messages if possible: these persist but PR descriptions do not. It’s okay to repeat information from such places, or simply to point to it. For one-commit PRs, Github will automatically populate the PR description with the commit message, so if you’ve written a good commit message you’re done! Sometimes there is "change-related" information that doesn’t belong in a commit message but is useful ("Kris I think this will fix the issue you had yesterday").
-
Review the diff of your own PR at the last minute before hitting "create". It’s amazing how many obvious things you spot here, and it stops the reviewer having to point them all out.
-
It’s fine to make WIP PRs if you just want to show your code to someone else or have the CI check it. Use the Github "draft" feature for this.
Until a PR is merged, the branch is yours to do with as you will. In particular, rebasing and force-pushing is fine. Indeed, if you need to update your branch with changes from master, rebasing is typically better than merging.
So please do use this ability where it helps, for example:
-
Add low-effort or WIP commits to fix review comments, and then squash them away before merging the PR.
-
If you have already had a PR review, don’t rebase away the old commits until the PR is ready to merge, so that the reviewer only has to look at the "new" commits.
-
Rewrite the commits to make the story clearer where possible.
-
Always prefer
git push --force-with-leaseto justgit push --forceto ensure that no work gets accidentally deleted.
Don’t be obsessive about history though: a little bit of effort making the history clear is nice, but you can rapidly hit diminishing returns. Use your judgement, but probably don’t merge a PR that has commits called "WIP" or "fix"!
If a PR is just a total mess, consider using Github’s squash-merge feature.
All pull-requests should be approved by at least one other person. We don’t enforce this, though: a PR fixing a typo is fine to self-merge, beyond that use your judgement.
As an author, code review is an opportunity for you to get feedback from clear eyes. As a reviewer, code review is an opportunity for you to help your colleagues and learn about what they are doing. Make the best use of it you can!
-
Pick the right reviewer(s). If you don’t know who to pick, ask!
-
Respect your reviewers' time. Their time is as valuable as yours, and it’s typically more efficient for you to spend time explaining or clarifying something in advance than for them to puzzle it out or pose a question.
-
If someone had to ask about your code, it wasn’t clear enough so change it or add a comment.
Read this blog post for more good tips: https://mtlynch.io/code-review-love/
-
Respond to review requests as quickly as you can. If you can’t review it all, say what you can and come back to it. Waiting for review is often a blocker for other people, so prioritize it.
-
If you don’t understand something, ask. You are as clever as any person who will read this in the future, if it confuses you it’s confusing.
-
Do spend the time to understand the code. This will help you make more useful comments, help you review future changes more easily, and help you if you ever need to work on it yourself.
-
More reviewing is usually helpful. If you think a PR is interesting, you can review it even if nobody asked you to, you will probably have things to contribute and you’ll learn something.
Read these blog posts for more good tips: - https://mtlynch.io/human-code-reviews-1/ - https://mtlynch.io/human-code-reviews-2/
We use Github actions to run the CI tests.
The CI will report statuses on your PRs with links to the logs in case of failure. Pull requests cannot be merged without status checks being green.
CI checks are run on the tip of the PR branch, not on the merge commit that is created with master. As a result, it’s possible to create a "semantic" merge commit where the CI passes on commits C1 and C2, but not on the merge of C1 and C2. In this circumstance we can end up with the CI checks being broken on master. However, this is sufficiently infrequent that we just live with the possibility, since eliminating it is quite awkward.