Personally, I prefer to specify command lines flags as an array rather than a string.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rARC30ecf46c1167: Allow `ArcanistExternalLinter` flags to be specified as an array.
arc unit
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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(). | |
Comment Actions
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. | |
Comment Actions
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.
Comment Actions
Disregard that... it seems that csprintf('%C %Ls', 'ruby', array('-w', '-c')) works whilst csprintf('%C %Ls', 'ruby', array('-w -c')) does not.
Comment Actions
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? | |
Comment Actions
This has been idle for a while, so it may need to be rebased. Let me know if you want me to do this.
Comment Actions
Oh, my inline is wrong -- the flags are part of the version.
This applies cleanly so I think we're good as-is.
Comment Actions
Closed by commit rARC30ecf46c1167 (authored by @joshuaspence, committed by @epriestley).