Page MenuHomePhabricator

Detect trailing spaces and tabs
ClosedPublic

Authored by johnny-bit on May 2 2017, 5:49 PM.

Details

Summary

Ref T12655 - this change properly detects trailing spaces or tabs (or combinations of thereof) on end of lines.

Test Plan

Use Text lint with trailing whitespace rule on files with spaces, tabs or combinations of thereof. Should properly detect and fix all those.

Diff Detail

Repository
rARC Arcanist
Branch
text-lint-tab (branched from master)
Lint
Lint Skipped
Unit
Tests Passed
Build Status
Buildable 16764
Build 22365: arc lint + arc unit

Event Timeline

These tests seem to pass on my system -- can you describe the \r\n issue you're running into in more detail? (Or just include the test case which isn't working, if it isn't one of these?)

I had 4th test file for cr/lf lineendings but couldn't get past

In 'trailing-whitespace-4.lint-test', expected lint to raise autofix on line 1 at char 28, but no autofix was raised. Actually raised:
  error at line 1, char 1: TXT1 DOS Newlines
  error at line 1, char 28: TXT2 Tab Literal

Whole file was identical to trailing-whitespace-3.lint-test but 3 first lines had cr/lf line endings (last one left only lf to properly detect ~~~~~~~~~~ plus had 2 error lines:

error:1:1
error:1:28

but generally seems to work while preserving line endings.
If you'd like I can send testfile to paste or add to this diff for checking.

Yeah, just go ahead and add it to the diff.

This is significantly involved because ArcanistLintPatcher, and other callsites elsewhere, hard-code "\n" line endings (e.g., in AcanistLintPatcher->getCharacterOffset()). This essentially prevents files with "\r\n" line endings from being patched correctly, at least in cases where patches interact with line endings.

I spent ~10 minutes poking at it without much success.

Let's not poke it further then. May we agree to remove trailing-whitespace-4.lint-test and say that "\r\n" line endings are generally no-go? This would slash troubleshooting this and all other callsites that have hardcoded line endings 😉

Remove failing test case due to hardcoded line endings "\n". This means that "\r\n" line endings are generally no-go.

Seems reasonable to me, thanks!

This revision is now accepted and ready to land.May 3 2017, 10:27 AM
This revision was automatically updated to reflect the committed changes.