Skip to content
Jeff Lunt edited this page Jan 15, 2015 · 24 revisions

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

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, or at least used as a starting place for a patch. The basic github contribution workflow applies:

fork -> patch -> submit pull request

Some general advice:

  • Keep your pull requests small and focused whenever possible. It's better to create more, smaller pull requests that make manageable, incremental improvements, than to introduce fewer, larger pull requests that require more time to audit and review. It you're working on a large part of the code, try to break it down into smaller chunks of work that can be safely integrated into the app as it grows.
  • Follow standards and conventions for the language (Ruby) and framework (Rails)
  • Leave the code better than you found it.
  • Think about the future, and try to keep the code current (with language and framework recommendations) when possible.
  • Don't use the Gemfile as a way to avoid writing some code. Introducing "quick solution" gems to trivial problems will only cost maintenance time down the road, so please consider adding gems with care.

Easy stuff:

If you're looking for something easy, but valuable, to do on the project here's a list of things we would love to get contributions on that are necessary, but not necessarily top priority, so they tend not to be the most urgent things. However, if a great pull request came along for them, we'd be likely to embrace it:

  • Conversion to haml: The project's views are a mix of haml and ERB. They will eventually all be haml, so any view that you want to convert would be much appreciated.
  • Test coverage: 100% coverage isn't the goal, but that said there are lots of gaps and inconsistencies in style. If you're looking to cleanup some model code, this set of tests is the style we'd like to see going forward. The reason this style of testing is preferred right now is:
    • The tests and rspec contexts are named after the methods they cover (unit tests, for the most part)
    • The sub-tests under the rspec contexts tell which conditional paths and edge cases are explicitly covered in that test
    • Overall this leads to making it more clear what things are covered, and perhaps more importantly what things are not currently covered by the test suite. This is critical for the long-term, because one must always think about future maintainers when considering the total cost of maintenance for an app.
  • Inconsistencies in naming: We have a Program model, but commonly refer to it as a Sponsor in our communications and in the UI. This is just one example, but this model really ought to be renamed to Sponsor, and supporting controller/view code changed as well.
  • Re-branding of app to NITROCompetitions: Anywhere in the code that you see the "NUCATS Assist" name is an opportunity to change it. Some examples run deeper than others, and some are trivial to change while others could turn into a multi-day mini project.
  • Wordiness in the UI: There are many places in the UI where an abundance of words could be replaced with a more concise explanation, or no words at all if an icon could replace it.
  • Partials that don't need to be partials: Generally view partials in Rails are reusable components. In many places in the app, however, they seem to be used to break up the view code into multiple files. This creates needless jumping from one file to another when the entire could really be in a single file. So, unless there's a good argument to be made for breaking a view into reusable parts, a single view is generally preferred.

Suggestions and pull requests in these easy areas are very much appreciated.

Clone this wiki locally