Page MenuHomePhabricator

Modify `ArcanistTextLinter` to respect `.editorconfig`
Needs RevisionPublic

Authored by joshuaspence on Jan 21 2015, 7:56 AM.
Referenced Files
F13310342: D11458.id31080.diff
Mon, Jun 10, 12:19 PM
F13307206: D11458.id27550.diff
Sun, Jun 9, 3:06 PM
F13303909: D11458.diff
Sat, Jun 8, 7:33 AM
F13288582: D11458.diff
Tue, Jun 4, 10:39 AM
F13287213: D11458.id31062.diff
Tue, Jun 4, 8:28 AM
F13253829: D11458.diff
Sat, May 25, 2:58 AM
F13249202: D11458.id27543.diff
Fri, May 24, 7:40 AM
F13249175: D11458.id27550.diff
Fri, May 24, 7:21 AM
"Love" token, awarded by adamchainz."Love" token, awarded by tycho.tatitscheff."Love" token, awarded by johnny-bit.



Fixes T5105. Modify ArcanistTextLinter to respect .editorconfig. The general idea is to consider .editorconfig files by default (maybe we can provide a text.ignore-editorconfig option, but I'm not sure there is much value in this). When linting a file, use PhutilEditorConfig::getConfig($path, $key) to determine which linter rules to apply (and in some cases how to apply them). If there is no .editorconfig file found, then resort to using the default values (i.e. the current behavior of ArcanistTextLinter). Depends on D9678.

Test Plan

This mostly works, but likely needs some polish.

Diff Detail

rARC Arcanist
Lint Passed
Advicesrc/lint/linter/ArcanistTextLinter.php:180XHP16TODO Comment
Advicesrc/lint/linter/ArcanistTextLinter.php:205XHP16TODO Comment
Test Failures
Build Status
Buildable 6097
Build 6117: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

12 msArcanistTextLinterTestCase::testLinter
1,186 msArcanistCSSLintLinterTestCase::testLinter
215 msArcanistCSSLintLinterTestCase::testVersion
25 msArcanistChmodLinterTestCase::testLinter
116 msArcanistClosureLinterTestCase::testLinter
View Full Test Results (1 Failed · 48 Passed · 3 Skipped)

Event Timeline

joshuaspence retitled this revision from to Modify `ArcanistTextLinter` to respect `.editorconfig`.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

FWIW, this mostly works but I'm going to hold off on spending too much time on this until after D9678.

I'd love to see this in mainline one day :-)


Shouldn't this be basically inverse of space for indentation?


This error should depend on indentation type set by editorconfig, right? So in case of tabs for indentation it should suggest tabs and not spaces.


Some projects do use BOM and require it in the source. EditorConfig project dis-encourages use of BOM, but nevertheless respects it.

I'd recommend to do something like:

case 'utf-8-bom':
  $checkbom = true;
case 'utf-8':
  $charset = 'UTF-8'; 	

And add one additional check for BOM after successful mb_check_encoding if UTF-8 check succeeded.

epriestley edited edge metadata.
epriestley added inline comments.

I think we should still default to 80, using a sentinel if that's easiest.


(Particularly since 80 is still documented as the default behavior.)


This won't be reached because we return from the 'tab' branch above.

This revision now requires changes to proceed.Apr 6 2015, 1:42 PM
joshuaspence edited edge metadata.
joshuaspence marked 4 inline comments as done.

Default to 80 character line length

joshuaspence edited edge metadata.

Use class constants from D12913

  • Fix default value for getEditorConfig
  • Tweak method visibilities
  • Tweaking
avivey added inline comments.

Would it be too preachy to default to lf here?


This doesn't look right...

maybe something like

'\r([^\n])' => '\r\n\1',
'([^\r])\n' => '\1\r\n',

(I'm pretty sure that's the wrong syntax for it)

epriestley edited edge metadata.

The newline stuff looks wrong to me -- notably lfcr ("Acorn BBC and RISC OS spooled text output.") is probably really crlf ("Windows, ..."). That is, "\r\n" is used on Windows, but this is "CRLF", not "LFCR".

I believe "cr"/"\r" is not a setting that is usable with any modern VCS tool, so it might be worthwhile to just throw rather than trying to support it.
I believe "lfcr" is also not usable either, and hasn't been used anywhere by anyone in the last 45 years.

Basically, I think there are only two real alternatives here: "crlf" (windows) and "lf" (everywhere else).

Also I typed "cr" and "lf" too many times there and probably got at least one wrong.


Should probably also do "\n" => "\r"?


This is sort of right, modulo @avivey's comment, except that it's consistent with "lfcr" (unused anywhere) and should probably be "crlf" (windows).


Note that phutil_split_lines() only recognizes "\n" (lf, unix) and "\r\n" (crlf, windows) newlines, because no VCS recognizes "\r" newlines. So this probably won't work with "cr" newlines. However, I believe nothing uses them.


e.g., called lfcr but string is crlf

This revision now requires changes to proceed.May 19 2015, 1:50 PM