I got this question on twitter and wanted to touch on it here.
Discourse accept the vast majority of pull requests we receive. We love all the contributions and thank you all for being so awesome.
Recently however we rejected the following PR
It was not that the PR was bad, most of it was good, its just that it became a big burden to take it from good to the state that everybody is happy with it.
Style is a very complicated topic. Certain things are completely non controversial.
# worse def hello result = "hello" end # better def hello "hello" end # worse def self.(name) unless g = lookup_group(name) g = refresh_automatic_group!(name) end g end # much better def self.(name) lookup_group(name) || refresh_automatic_group!(name) end
Certain things are a little bit more open to interpretation:
# which is better? string.upcase! unless string.blank? string.upcase! unless string.nil? string.upcase! if string.present? string.upcase! if string # I much prefer the last but its taste thing # which is better? something if Rails.env == "development" || Rails.env == "profile" something if Rails.env.development? || Rails.env == "profile" # which is better? products.where('deleted_at IS NULL') products.where(:deleted_at => nil)
As a rule, we always prefer PRs with substance
- Add / refactor a few tests AND improve surrounding code.
- Add some new functionality, improve surrounding code.
- Highlight a new interesting pattern we should be using
- Do some “real” refactoring, break up a big class, extract the onebox gem, and so on.
We also accept PRs that address egregious style crimes:
lookup_group(name) || refresh_automatic_group!(name)change.
- clean up some of the mess left from the coffeescript -> js move
We are very worried about huge PRs that enforce a certain style, the famous remove all trailing white spaces or for example walking the entire code base to enforce a “taste” style change. Its tricky, we don’t want to discourage contributions.
If you are looking for projects we try to keep: Discourse Development Contribution Guidelines up to date.
I would also like to open this whole topic to discussion, and ideas on what you think should be our best approach.