Page MenuHomePhabricator

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

Authored by joshuaspence on Jan 21 2015, 7:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 24, 2:49 AM
Unknown Object (File)
Thu, Nov 24, 2:49 AM
Unknown Object (File)
Thu, Nov 24, 2:49 AM
Unknown Object (File)
Thu, Nov 24, 2:48 AM
Unknown Object (File)
Thu, Nov 24, 2:48 AM
Unknown Object (File)
Thu, Nov 24, 2:48 AM
Unknown Object (File)
Thu, Nov 24, 2:48 AM
Unknown Object (File)
Thu, Nov 24, 2:48 AM
Tokens
"Love" token, awarded by adamchainz."Love" token, awarded by tycho.tatitscheff."Love" token, awarded by johnny-bit.

Details

Summary

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

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

Unit TestsFailed

TimeTest
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 :-)

src/lint/linter/ArcanistTextLinter.php
181–182

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

192–193

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.

203–207

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'; 	
 break;

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.
src/lint/linter/ArcanistTextLinter.php
19

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

40–42

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

192–193

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.
src/lint/linter/ArcanistTextLinter.php
131

Would it be too preachy to default to lf here?

149

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.

src/lint/linter/ArcanistTextLinter.php
142

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

149

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

237

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.

431–432

e.g., called lfcr but string is crlf

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