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.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5105: Ensure that `ArcanistTextLinter` respects `.editorconfig` settings
This mostly works, but likely needs some polish.
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed Severity Location Code Message Advice src/lint/linter/ArcanistTextLinter.php:180 XHP16 TODO Comment Advice src/lint/linter/ArcanistTextLinter.php:205 XHP16 TODO Comment - Unit
Test Failures - Build Status
Buildable 6097 Build 6117: [Placeholder Plan] Wait for 30 Seconds
| Time | Test | |
|---|---|---|
| 12 ms | ArcanistTextLinterTestCase::testLinter | |
| 1,186 ms | ArcanistCSSLintLinterTestCase::testLinter | |
| 215 ms | ArcanistCSSLintLinterTestCase::testVersion | |
| 25 ms | ArcanistChmodLinterTestCase::testLinter | |
| 116 ms | ArcanistClosureLinterTestCase::testLinter | |
| View Full Test Results (1 Failed · 48 Passed · 3 Skipped) |
Event Timeline
Comment Actions
FWIW, this mostly works but I'm going to hold off on spending too much time on this until after D9678.
Comment Actions
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. | |
Comment Actions
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 | |