Page MenuHomePhabricator

Added RuboCop linter
ClosedPublic

Authored by cburroughs on Oct 22 2014, 5:43 AM.

Details

Test Plan

Add the following JSON to your .arclint:

"rubocop": {
  "type": "rubocop",
  "include": "(\\.rb$)"
}

Then, add a ruby file with errors, for instance:

def hello()
  puts 'world'
end

Run arc lint. It should come up with something along the line of: "Omit the parentheses in defs when the method doesn't accept any arguments."

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

remon updated this revision to Diff 25775.Oct 22 2014, 5:43 AM
remon retitled this revision from to Added RuboCop linter.
remon updated this object.
remon edited the test plan for this revision. (Show Details)
remon added a comment.Oct 22 2014, 9:18 PM

I'll try and fix the XHPAST errors and get some linting/unit results in.

remon planned changes to this revision.Oct 22 2014, 9:18 PM
remon updated this revision to Diff 25783.Oct 22 2014, 9:37 PM
remon edited edge metadata.

Fixed typo, lint errors.

chad added a subscriber: chad.Oct 22 2014, 10:39 PM

omg it comes with unit tests.

avivey awarded a token.Nov 2 2014, 7:06 AM
jjooss added a subscriber: jjooss.Dec 2 2014, 4:04 PM

polite bump

joshuaspence accepted this revision.Dec 30 2014, 9:28 AM
joshuaspence edited edge metadata.

FWIW, this looks mostly fine to me.

src/lint/linter/ArcanistRuboCopLinter.php
93

Prefer phutil_json_decode or, at the very least, json_decode($stdout, true).

117–132

This method seems a little messy. I'd probably just move it inline.

remon updated this revision to Diff 26566.Dec 30 2014, 10:39 AM
remon edited edge metadata.
  • Inline severity method
  • Associative arrays instead of objects!

Just swinging by as an interested observer, @remon it looks like you still need to sign the CLA.

remon added a comment.Jan 12 2015, 1:32 PM

Just swinging by as an interested observer, @remon it looks like you still need to sign the CLA.

I already did that, I think that this diff is ready to be landed!

joshuaspence added inline comments.Jan 19 2015, 7:46 AM
src/lint/linter/ArcanistRuboCopLinter.php
46

Prefer to write this as pht('Install RuboCop using %s`, 'gem install rubocop') because 'gem install rubocop'` isn't really translatable.

49–51

After D11322, this is the default behavior.

53–55

This is the default behavior, so you can just omit this method entirely.

src/lint/linter/__tests__/ArcanistRuboCopLinterTestCase.php
4–5

After D11197, this should extend from ArcanistExternalLinterTestCase.

8–10

After D11172, this should just be $this->executeTestsInDirectory(dirname(__FILE__).'/rubocop/');

@remon sorry to poke you again, are you still working on joshuaspence's inline comments? If not I can take a stab at commandeering.

remon added a comment.Feb 10 2015, 1:11 AM

That's ok! I haven't found the time to work on this yet. Feel free to commandeer!

cburroughs commandeered this revision.Feb 10 2015, 2:48 AM
cburroughs added a reviewer: remon.
cburroughs updated this revision to Diff 28242.Feb 10 2015, 2:54 AM
  • updats for review feedback and changing test names
  • satisfy the linting gods
  • I updated the library map...

Some minor feedback.

src/lint/linter/ArcanistRuboCopLinter.php
46

This looks pretty awkward, try pht('Install RuboCop using `%s`.', 'gem install rubocop') instead.

52

I think that maybe it would be better (or, at least, more consistent) to remove this flag and to, instead, use the .arclint file to manage exclusions.

53

Probably not required with --format=json

90–96

Prefer to write this by chaining the set* methods together.

cburroughs added inline comments.Feb 11 2015, 7:58 PM
src/lint/linter/ArcanistRuboCopLinter.php
46

Done.

52

This flag is confusing https://github.com/bbatsov/rubocop/issues/893

With this flag: rubocop exclusions win over arclint inclusions.

Without this flag: rubocop exclusions won't matter for the purposes of arc lint.

So there is probably some complicated case where it is useful, but I agree it's probably more consistent with the rest of arcanist to remove.

53

Done.

90–96

Done.

cburroughs updated this revision to Diff 28309.Feb 11 2015, 7:58 PM
  • fancier pht games
  • color setting redundant with json format
  • idiomatic message chaining
  • goodbye confusing flag

@joshuaspence I think you already have the accepted flag set, but does this look good to you for realz now?

In case if my project already contains a .rubocop.yml, will linter leverage this configuration?

rubocop.conf can be used to point at whichever configuration file you like.

cburroughs added a subscriber: reu.Mar 30 2015, 6:04 PM
cburroughs planned changes to this revision.Apr 6 2015, 12:17 PM

Minor severity-mapping updates on the way.

cburroughs updated this revision to Diff 29533.Apr 6 2015, 4:09 PM
  • avoid calling setSeverity directly
  • handle all rubocop severity values
epriestley accepted this revision.Apr 6 2015, 4:10 PM
epriestley edited edge metadata.
epriestley added inline comments.
src/lint/linter/ArcanistRuboCopLinter.php
103–106

Prefer docblock style.

This revision is now accepted and ready to land.Apr 6 2015, 4:10 PM
This revision was automatically updated to reflect the committed changes.