Page MenuHomePhabricator

Added RuboCop linter
ClosedPublic

Authored by cburroughs on Oct 22 2014, 5:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 1, 5:44 PM
Unknown Object (File)
Sun, Jun 26, 9:33 PM
Unknown Object (File)
Sun, Jun 26, 1:58 PM
Unknown Object (File)
Sun, Jun 26, 11:34 AM
Unknown Object (File)
Sun, Jun 26, 8:17 AM
Unknown Object (File)
Sun, Jun 26, 8:13 AM
Unknown Object (File)
Sun, Jun 26, 12:26 AM
Unknown Object (File)
Sat, Jun 25, 7:43 AM
Tokens
"Mountain of Wealth" token, awarded by avivey.

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
Lint Not Applicable
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
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 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
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.

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

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.

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