-
Notifications
You must be signed in to change notification settings - Fork 25
Pull Request Guidelines
#Guidelines for submitting PRs
When you submit a PR, it will be reviewed by CSE and/or your peers. That review process will check your code for correctness and standards compliance, and is an on-going process of comments, critique and commits. During this process, you can push new commits to your branch (correcting issues raised, making changes) and these will update the PR automatically.
Once everyone is happy and the PR is ready to merge, you may be asked to squash your commits into one or more sensible commits. This happens when there are a lot of small edits often with not really useful commit messages, for example
- merged
- line endings
- white space
- fix null reference
Squashing, or rebasing your branch to have one main commit, keeps the history of the target repository nice and clean.
Ah, coding standards. Love 'em, or hate 'em they exist and sometimes we have to adhere to them. When submitting PRs to CSE repositories they must conform to the coding standards defined by CSE, otherwise the PR quite simply will not be accepted.
We currently use the Airbnb JavaScript and CSS Style Guides for our standards. In time we may produce our own modified version of these standards, but for now it is straight up airbnb.
CSE Javascript Style Guide (based on Airbnb Javascript Style Guide)
I am old school, came from a Unix/C and vi(1) environment, where the standard was to use \t (tab) characters for indent. This was important back then because disk space was a premium. Disk packs where the size of buildings and had about 1 megabyte of storage on them (I may be exaggerating slightly). It also meant the developer could choose if they wanted 4 space or 8 spaces to represent a tab (8 space tab indented code eew!).
I have used 4 space tabs ever since those days. I have seen the arguments for using spaces instead of tabs and for having 2 space indents rather than 4 or 8 and managed to avoid them, until now.
So I start coding for CSE and I am told we are using 2 space tabs. At first I was horrified, how could I possibly change the habit of a lifetime (and that almost isn't exaggerating, I have been using 4 space tabs for 30 odd years), but I accept if I want to contribute to the project there needs to be a standard otherwise the code will look a right mess. So I reluctantly changed the settings in my editor to use 2 space indents and to replace tabs with spaces. And you know what? I am ok with it now, so much so I have even considered switching to it at work, but then I have millions of lines of code all using 4 space tabs, so probably not a good idea.
The reason for standards is simple. In an environment where there are multiple contributors be that at work or over the internet, it is important to keep a consistent style across all the source code. We each have our own individual way of doing things, and if we allowed everyone to do whatever they like in the code, the code would look messy, very messy. Imagine if I like 2 space indents and you like 8 space tabs, and I edit some of your code, and stick with my 2 space indents because well, there are no rules saying I can't. The code would quickly become unreadable and difficult to maintain, which leads to errors and poor quality software. Standards are there to ensure the code remains consistent, and that anyone in the team or new to the team has the best possible chance of understanding the code quickly without having to fight difficult to read code.
Why are you so strict at enforcing standards? That extra space makes no difference to the code I say!!!
Ask yourself this. If we have a set of rules, who decides which ones are important and must be obeyed and which ones can be ignored? The individual submitting the PR? Then we may as well have no standards at all. The reviewer(s) of the code? Then we run into trouble with disagreement over which rules should be enforced and which ones should not.
The only way to work with standards is to assume all rules are mandatory unless the standards explicitly says otherwise. As a reviewer that is the only sensible way we can review code. All coding standards must be obeyed, no exceptions. That is not nit-picking it is just being pragmatic, it's the only way standards can sensibly work.
Initially, it's tough. The standards as they exist at the time you submit the PR are gospel, don't follow all the rules, the PR will not be accepted. However, if you feel strongly enough about a particular rule being silly, pointless or that there is a better way, then open an issue for discussion.
CSE_JB has talked about producing a modified version of the airbnb javascript standard, but until we have such a document, the airbnb standard is what we follow, regardless of any rules you may not like.
Challenge it. We all make mistakes, and we may interpret or apply the standards incorrectly. You will not be thought bad of for challenging a review even if ultimately we decide you are wrong.
We would rather you don't ignore them, but discuss, even if that is simply to disagree with the comment, at least consider what the reviewer has said or asked, they may even learn something from your response.
Your code will not be merged.