Details

Reviewers

remon
joshuaspence
epriestley

Group Reviewers

Blessed Reviewers

Commits

rARC41ddd34aeb26: Added RuboCop linter

Required Signatures

L28 Phacility Individual Contributor License Agreement

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

Branch

rubocop

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 5157
Build 5175: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed
View All

	Time	Test
	50 ms	ArcanistRubyLinterTestCase::testLinter
	27 ms	ArcanistChmodLinterTestCase::testLinter
	0 ms	ArcanistClosureLinterTestCase::testVersion
	13 ms	ArcanistCommitLinterTestCase::testLinter
	0 ms	ArcanistCpplintLinterTestCase::testVersion
		View Full Test Results (1 Failed · 27 Passed · 28 Skipped)

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 reviewers: Blessed Reviewers, epriestley.

Herald edited edge metadata. · View Herald TranscriptOct 22 2014, 5:43 AM

Herald added a required legal document: L28 Phacility Individual Contributor License Agreement. · View Herald Transcript

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

Harbormaster completed remote builds in B2876: Diff 25775.Oct 22 2014, 5:44 AM

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

Fixed typo, lint errors.

Harbormaster completed remote builds in B2881: Diff 25783.Oct 22 2014, 9:38 PM

omg it comes with unit tests.

joshuaspence added a reviewer: joshuaspence.Nov 2 2014, 4:12 AM

avivey awarded a token.Nov 2 2014, 7:06 AM

cburroughs added a subscriber: cburroughs.Nov 3 2014, 3:18 PM

polite bump

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.

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.

In D10738#105208, @cburroughs wrote:

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.

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.

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.

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?

calfzhou added a subscriber: calfzhou.Mar 13 2015, 4:35 AM

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

Minor severity-mapping updates on the way.

cburroughs mentioned this in D11657: Add a ShellCheck linter.Apr 6 2015, 4:01 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
102–105	Prefer docblock style.

This revision is now accepted and ready to land.Apr 6 2015, 4:10 PM

Closed by commit rARC41ddd34aeb26: Added RuboCop linter (authored by cburroughs, committed by epriestley). · Explain WhyApr 6 2015, 4:11 PM

This revision was automatically updated to reflect the committed changes.

getaaron mentioned this in T10605: Implement setOriginalText() and setReplacementText() in RuboCop driver.Mar 16 2016, 5:52 PM

Added RuboCop linter
ClosedPublic
Actions

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Revision Contents
Changeset List

Diff 29533

src/__phutil_library_map__.php

src/lint/linter/ArcanistRuboCopLinter.php

src/lint/linter/tests/ArcanistRuboCopLinterTestCase.php

src/lint/linter/tests/rubocop/convention.lint-test

src/lint/linter/tests/rubocop/error.lint-test

src/lint/linter/tests/rubocop/no_errors.lint-test

src/lint/linter/tests/rubocop/warning.lint-test

Added RuboCop linterClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Revision ContentsChangeset List

Diff 29533

src/__phutil_library_map__.php

src/lint/linter/ArcanistRuboCopLinter.php

src/lint/linter/__tests__/ArcanistRuboCopLinterTestCase.php

src/lint/linter/__tests__/rubocop/convention.lint-test

src/lint/linter/__tests__/rubocop/error.lint-test

src/lint/linter/__tests__/rubocop/no_errors.lint-test

src/lint/linter/__tests__/rubocop/warning.lint-test

Added RuboCop linter
ClosedPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List

src/lint/linter/tests/ArcanistRuboCopLinterTestCase.php

src/lint/linter/tests/rubocop/convention.lint-test

src/lint/linter/tests/rubocop/error.lint-test

src/lint/linter/tests/rubocop/no_errors.lint-test

src/lint/linter/tests/rubocop/warning.lint-test