Contributing code

(work in progress)

If your patch is good, well thought out, and fits in with the direction of the app, then there's a decent chance it'll be accepted. The basic github contribution workflow applies:

fork -> patch -> submit pull request

This guide is not required reading for contributors. It is intended to help you increase the probability that your pull request is accepted.

Here's some general advice:

• Follow standards and conventions for the language (Ruby) and framework (Rails)
• Leave the code better than you found it

General Ruby style

This is a list of things that are either pet peeves, or just don't seem to make much sense, and I'm going to pick on the project itself to point out where I think it can be improved.

• Put private methods all together in their own section at the bottom of the file. I know this rule is being broken all over the place, but this style is going away. I will happily accept pull requests that do nothing be cleanup files like this. This...is a mess.
• Code is code. English is English. Please don't cross the two by going bonkers with so-called "code readability" crusades. If you've done any significant work in a language whose designers think that code that reads like English is some kind of virtue, then I hope you've come away from that experience knowing the pain points. Code is (or at least should be) clear and unambiguous. Seeing a word in code that you don't understand (e.g. "idempotent") doesn't make it unclear, it just means you should lookup the definition of that word. What's unclear is when we use English words that have vague meanings in code.

Model style

Group and order things consistently

• Do more of this (notice how associations, accessors, callbacks, validations, and scopes are all neatly put together - also notice how all validations associated with a given column are put together)
• ...and less of this

Avoid callbacks when you can

Callbacks in ActiveRecord are supposed to make our lives easier; they are not supposed to turn nice, sequential code into a giant pile of event-driven garbage the complicates the process of reading and writing to the database so much that it seems impossible to get anything to validate successfully.

Choose your callbacks wisely, and only when really necessary.

Don't use default_scope unless you have a really, REALLY good reason to do so

Putting a default scope on models tends to hork things up supremely. It's one of those nice to have features that's intended for the special case in which they actually do great things, but instead gets abused all to hell because we think we're saving ourselves a few keystrokes across a few views by adding a default scope to all our models.

The problem is that default scopes have a way of obscuring what models are being returned by conventional ActiveRecord calls, and changing the meaning of things unnecessarily.

If you really think a default scope is good, try writing it as a named scope first. Explicit is better than "automagic" 99% of the time.

Don't try to be too clever

You don't need custom validation messages everywhere. This is an attempt to be clever and often adds to the amount of work it takes to maintain the code over the long-term. If you're adding a custom validation message for a field, first ask yourself, "Is this really necessary, or am I just rewording the default Rails validation message?"

Controller style

Stick with the default Rails controller actions for the most part (new, create, edit, update, delete, index). If you're thinking:

I think this model needs an "archive" action, so I'll create a new action called "archive" and add it to the associated controller.

Technically archiving is a type of updating, so put that code in the update action unless you are willing to go through the trouble of arguing to the contrary, knowing full-well that there will be disagreement.

View style

I know it doesn't look like it in most places, but the project is migrating to haml. If you see an ERB view hanging out there and want to convert it to haml, then I would be quite appreciative. This kind of cleanup is something that just needs to be done, yet isn't always a priority if the view is working as designed.