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 retitled this revision from to Added RuboCop linter.
remon updated this object.
remon edited the test plan for this revision. (Show Details)

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

remon edited edge metadata.

Fixed typo, lint errors.

omg it comes with unit tests.

joshuaspence edited edge metadata.

FWIW, this looks mostly fine to me.

src/lint/linter/ArcanistRuboCopLinter.php
92

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

116–131

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

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.

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!

src/lint/linter/ArcanistRuboCopLinter.php
45

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

48–50

After D11322, this is the default behavior.

52–54

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

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

After D11197, this should extend from ArcanistExternalLinterTestCase.

7–9

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.

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

cburroughs added a reviewer: remon.
  • 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
45

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

51

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.

52

Probably not required with --format=json

89–95

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

src/lint/linter/ArcanistRuboCopLinter.php
45

Done.

51

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.

52

Done.

89–95

Done.

  • 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.

Minor severity-mapping updates on the way.

  • avoid calling setSeverity directly
  • handle all rubocop severity values
epriestley edited edge metadata.
epriestley added inline comments.
src/lint/linter/ArcanistRuboCopLinter.php
102–105

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.