Defining rubocop style

[lloeki]

This PR initially contains an empty .rubocop.yml and a freshly-generated .rubocop-todo.yml at the root.

In this PR we can discuss about each cop (disabling, enabling as is or setting parameters and variants) possibly even straight in the todo file code. I'm not going to be picky, if a cop does not make sense, it gets disabled: no need to be too stringent as it would encourage bad code.

For each decision that is taken, the selected cop configuration is written to .rubocop.yml and removed from the todo file, updating the PR branch.

Once the todo file is empty, it gets removed and we have our target style documented in rubocop.yml. At this point the goal is not to have zero violations but to have a file that will support us into making code more conformant and uniform in later PRs either automatically with autocorrect or on a case by case basis, according to homebrew's management team policies and preferences.

From then on anyway, the mere presence of the file will entice people having rubocop installed and creating new files or edit existing files fix those files one by one as time goes by.

[lloeki]

Style/StringLiterals: This one has the most offenses. I seemed to notice the homebrew style is to prefer double quotes in most cases, except when too much escape sequences come in. This can be set up by using EnforcedStyle: double_quotes instead of the default.

[lloeki]

Default rubocop configuration can be seen here, including possible alternative settings for each cop.

[MikeMcQuaid]

We're using the Ruby Style Guide in case that helps at all.

[lloeki]

Most of Rubocop defaults follow that guide already.

It seems Library/Homebrew/cmd/doctor.rb can't decide whether it's indented or not inside the Check class, possibly to save two spaces given its length. I suggest indenting everything consistently, especially since two spaces are not worth it.

[lloeki]

Needed a nudge here, else the file was unparsable.

[lloeki]

After enforcing double quotes, 19k violations remain on that one. Many of them are C-style single-char strings.

[lloeki]

All of them are in the same file. Rubocop recommends using string delimiters instead (I didn't even know about this syntax)

[jacknagel]

It seems Library/Homebrew/cmd/doctor.rb can't decide whether it's indented or not inside the Check class, possibly to save two spaces given its length. I suggest indenting everything consistently, especially since two spaces are not worth it.

See https://github.com/Homebrew/homebrew/blob/290934c096c25482a89500c7ce1c95c7043e434c/Library/Homebrew/cmd/doctor.rb#L73; most of the code in that file predates the Checks class, so it wasn't reindented to avoid churn.

[lloeki]

Just a quick note that I'm still working on this, but I have a few deadlines not to miss.

[MikeMcQuaid]

Just FYI: I'm wanting to integrate something like this into our CI for new formulae/files so I'd be interested in pushing whatever's ready.

[lloeki]

OK, turns out I'll have some spare time tomorrow, so I'll dive into this again and push some stuff for you.

[jacknagel]

One thing that might help move this along quicker is if we only enable it on the formula directory for now. Is that something that can be done?

We have a pretty good idea of what formula style should look like, but the style guidelines for the "core" may differ somewhat, as it is more like actual application code than the domain-specific code in formulae.

Perhaps we can enable it for formulae now and worry about core later?

[MikeMcQuaid]

Yeh, I'm planning on setting it up so, initially at least, we're running it on individual (new) files rather than all of them.

[lloeki]

We have a pretty good idea of what formula style should look like, but the style guidelines for the "core" may differ somewhat, as it is more like actual application code than the domain-specific code in formulae.

@jacknagel that's totally possible to set up, we just need to have one .rubocop.yml file in Library/Formula and one in Library/Homebrew. If needed, they could inherit_from a common one in Library or at the root (like as of now) for common rules.

[lloeki]

I noticed this perl backref pattern:

    version_info = `#{emacs_binary} --version`
    version_info =~ /GNU Emacs (\d+)\./
    major = $1

There are only a few occurrences in formulas (vs ~90 in the core) and some of them may be dangerous to the untrained eye, or its intrinsic untested branching quickly overlooked. It could be useful to enforce the rule on Formula.

[lloeki]

Please note that due to a rubocop bug it may be needed to write # rubocop:disable before __END__, or to globally disable offending cops (currently Style/TrailingWhitespace and Style/Tab).

[lloeki]

Need your feedback on Style/TrailingComma. It's about this:

x = [
   "foo",
   "bar"
]
# vs
x = [
   "foo",
   "bar",     # final comma presence makes things easy to reorder, insert or delete
]

See gcc formula for a concrete example.

Ruby style guide is mute on that point, and currently it's about 40 vs 20 respectively. I personally prefer the latter style very much (for pragmatic reasons, as it's very editor friendly). What's your opinion on this?

[MikeMcQuaid]

Please note that due to a rubocop bug it may be needed to write # rubocop:disable before END, or to globally disable offending cops (currently Style/TrailingWhitespace and Style/Tab).

Let's just disable these.

Style/WordArray (i.e using %w(foo bar) instead of ["foo", "bar"]) has 18 violations among 429 total uses but is arguably worse in some of said violations. Given the ratio, do we enforce nonetheless to comply with the Ruby style guide?

I prefer the latter here, actually, as it's more obvious what it's doing to non-Ruby folks.

Need your feedback on Style/TrailingComma. It's about this:

Definitely prefer the trailing comma too.

[lloeki]

Enforcing SpaceAroundOperators is very useful for readability and recommended by the style guide. Yet it may conflict with the current use of / for concatenating paths, which seems to favour no spaces around. Autocorrecting gave me a few real-life examples and it's actually not that bad having spaces around, making the variables and concatenation points stand out a little more:

bin/"foo"
# vs
bin / "foo"  # seems striking at first, but mostly because we're not used to it

Disabling SpaceAroundOperators comes at the cost of people landing random space presence around operators, as each one seems (violations do show frequent inconsistencies, although spacing things is favored) to have its own preferences, or rather, tolerance. On the other side of the coin, no-space-around-/ looks very shell-ish. It really depends on whether you want it to appear like magic (and possibly opaque to non-ruby users) strings or slightly hint that it's actually a nice concatenation operator.

[lloeki]

There's no way currently to enforce forbidden use of %(). I've opened an issue upstream.

[MikeMcQuaid]

I prefer the prior wrt SpaceAroundOperators

[lloeki]

Pushed an update. As we said before, please disregard the formula fixes as only rubocop.yml will remain once I'm done.

[MikeMcQuaid]

This needs rebased on master. Please remove the formula fixes if you don't mind as it'll kick off a massive CI job that I'll need to kill (doing this now).

[lloeki]

Removed formula fixes, and squash-rebased rubocop files. I had to remove the simple inheritance thingy because it seems to make things a bit flaky since I named both levels .rubocop.yml, so I though since we're focusing on formulas, we might as well make things simple and clear both to us and to everyone else (including both tooling and formula contributors).

[lloeki]

Style/UnneededPercentX has ~400 occurences of backticks vs ~15 occurences of %x{}. I personally favor %x{} for multiline as stranded backticks just don't stand out enough, especially with some fonts and colors. A case can be made for or against on single line either for consistency, shell-likeness, chained method call readability or stringiness of result.

An issue is opened upstream to enforce %x{} in such cases (default is to enforce backticks everywhere).

[lloeki]

Style/SpecialGlobalVars warns about use of $?, $0 and $/, favoring explicit synonyms like $CHILD_STATUS, $PROGRAM_NAME, and $INPUT_RECORD_SEPARATOR or $RS respectively. The habit displayed in current formula seems to confirm this is not a problem, so I disabled it but feel free to say otherwise.

[MikeMcQuaid]

This can't live in the Formula directory, unfortunately, as everything in there is treated as a formula. Let's just do a single file in Library instead.

[lloeki]

OK I'll move it up, although please note it'll apply to the whole tree. I'm surprised there's no globbing though: what if I happen to have a go.rb~ or .DS_Store in there?

[MikeMcQuaid]

There's a bunch of known issues with this but it is what it is for now, sadly.

[MikeMcQuaid]

May as well consolidate the two in this case: I'm not convinced we need separate rules (for now) for Library/Formula and Library/Homebrew.

[jacknagel]

The core of Homebrew ignores non-.rb files, but things like test-bot that read paths from diff output do not.

[lloeki]

Lint/ParenthesesAsGroupedExpression warns about ~20 priority-forcing, off-putting syntax:

# excerpts:
assert (output_dir/"url.txt").read.include?("http://brew.sh")
rm_rf (couchdb_share/"www/script/test/geocouch")
assert (Pathname.pwd/"foo").read.include? "hello homebrew!"
inreplace (share/"geocouch/geocouch.plist"), "%bindir%/%couchdb_command_name%", \
cd ("yara-python") do
cd (buildpath+"Ghcsource/tests") do

I can understand the will of consistently not using parentheses for method calls and spacing things like we're the shell, but from an outsider point of view not intimate with ruby's parser, this can be highly confusing, and given the low number of occurences I'd favor leaving the cop active. What's your take on this one?

[lloeki]

Style/AlignParameters wants to change things this way:

     system "make", "-C", "ace", "-f", "GNUmakefile.ACE",
                    "INSTALL_PREFIX=#{prefix}",
                    "LDFLAGS=",
                    "DESTDIR=",
                    "INST_DIR=/ace",
                    "debug=0",
                    "shared_libs=1",
                    "static_libs=0",
                    "install"
# into
     system "make",
            "-C", "ace", "-f", "GNUmakefile.ACE",
            "INSTALL_PREFIX=#{prefix}",
            "LDFLAGS=",
            "DESTDIR=",
            "INST_DIR=/ace",
            "debug=0",
            "shared_libs=1",
            "static_libs=0",
            "install"

Do you think the second style is suitable for formulas or should I disable the cop (created an issue upstream)? It sounds sensible to have system be a special case.

[MikeMcQuaid]

Yeh, disable the second cop. With parens I don't think we want to consistently enforce them and the errors when they are ambigious are relatively obvious. We can always tweak these things later, thanks!

[lloeki]

Style/PercentLiteralDelimiters raises ~300 violations when respecting the style guide (i.e use () except for %r), mostly made of inconsistent %W and %w. The most used style for wW seems to be [], followed by {}, and finally the style-recommended (). When electing to enforce [] (which IMHO looks best, in a tie with (), especially next to often-used string interpolations), this is down to about 140, so I've set it this way, and you can always tweak it to something else later.

[lloeki]

This last push marks the completion of the analysis from the formulas, moves .rubocop.yml in Library, and removes .rubocop_todo.yml together with its inclusion form inside .rubocop.yml. What remains if you run rubocop Formula from Library is actual violations (pending tweaks).

[lloeki]

For a little bit of statistics, this totals 17262 violations, of which 13644 are double quotes and 2858 are line length, the remainder being a long tail of small, inconsequential violations mostly counted in the single (or barely double) digits, that will be easily fixed one formula at a time.

[MikeMcQuaid]

Thanks for the work here @lloeki! Will try and get this hooked up when I get the chance.

[DomT4]

Neat. This is awesome. I presume we can just gem install rubocop and rubocop in the formula directory and it'll generate a report based on the new ruleset?

[MikeMcQuaid]

Yep. I'm going to tweak it a bit further though.

[lloeki]

Yes gem install rubocop will do. Remember that rubocop is regularly updated, so it's best to have a 'latest version only' policy and rerun this from time to time (if this is a problem we can talk about Gemfiles). rubocop [path or file, or ommited] will output violations on stdout, while editors plugins (like syntastic for vim or linter+linter-ruby+linter-rubocop for Atom) will highlight violations straight in the editor, which will be very useful (and used to be very annoying :-)) to formula authors that also happen to have the plugins. Please note that to generate a file, you have to use rubocop --auto-gen-config and since this currently does not take an argument, it will output rubocop_todo.yml for the whole Library tree. A workaround is to move .rubocop.yml into Formula and run the command from there.

[jasonmp85]

As someone who once flippantly asked "why am I being told that I need to use double quotes for everything instead of your project having a rubocop file?" I am happy to see Homebrew finally get a rubocop file 👍.

[MikeMcQuaid]

As someone who once flippantly asked "why am I being told that I need to use double quotes for everything instead of your project having a rubocop file?" I am happy to see Homebrew finally get a rubocop file .

A Rubocop file that'll fail submissions of new formulae that don't meet the style guidelines, too. Glad to hear you appreciate it, thanks @jasonmp85 and @lloeki for getting us there!