Page MenuHomePhabricator

Allow `ArcanistExternalLinter` flags to be specified as an array.
ClosedPublic

Authored by joshuaspence on Mar 3 2014, 12:28 AM.

Details

Summary

Personally, I prefer to specify command lines flags as an array rather than a string.

Test Plan

arc unit

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This possibly needs more work, but I have submitted to get some initial feedback.

epriestley edited edge metadata.

Yeah, this is really good. I generally lean toward being more aggressive with it and deprecating string support, which is unsafe/ambiguous/yuck. This stuff is mostly fairly new, and probably has little use outside of the upstream. But whatever you're comfortable with is fine; less aggressive versions of this patch put us in a better place, too.

Couple of minor inlines.

src/lint/linter/ArcanistExternalLinter.php
18

I think the old version of this may have been slightly more clear.

355–370

It would be nice if we could return array() here and then use %Ls and avoid any of this %C stuff. I'm not sure if any linters would have trouble adapting to that or not. I think it would be OK to break the .arclint format, even, and require list<string>.

357

I think this should be nonempty() now instead of coalesce().

This revision now requires changes to proceed.Mar 4 2014, 7:10 PM

I'd prefer to be more aggressive and deprecate string support as much as possible. I will resubmit this diff the way that I would like it (ideally), and we can add deprecation warnings where necessary.

src/lint/linter/ArcanistExternalLinter.php
18

Whoops. Not sure how this happened.

355–370

I agree with this.

Hm... Interestingly, ruby '-w -c' (which is the output from csprintf('%C %Ls', 'ruby', array('-w', '-c')) is not equivalent to ruby -w -c.

On my machine, it causes an error:

> ruby '-w -c'
ruby: invalid option -   (-h will show valid options) (RuntimeError)

I think that we should have a %LR format specifier for csprintf.

Disregard that... it seems that csprintf('%C %Ls', 'ruby', array('-w', '-c')) works whilst csprintf('%C %Ls', 'ruby', array('-w -c')) does not.

joshuaspence updated this revision to Unknown Object (????).Mar 5 2014, 9:09 PM

Be more strict.

This revision now requires changes to proceed.Apr 23 2014, 11:18 PM
epriestley edited edge metadata.

Sorry, missed these updates somehow. One minor inline but I can fix that in the pull if it looks OK?

src/lint/linter/ArcanistPEP8Linter.php
19–20

This shoudl be %C --version %Ls now?

This revision is now accepted and ready to land.Apr 23 2014, 11:21 PM

This has been idle for a while, so it may need to be rebased. Let me know if you want me to do this.

Oh, my inline is wrong -- the flags are part of the version.

This applies cleanly so I think we're good as-is.

epriestley updated this revision to Diff 20998.

Closed by commit rARC30ecf46c1167 (authored by @joshuaspence, committed by @epriestley).