Page MenuHomePhabricator

Update LESSC linter
Needs ReviewPublic

Authored by avivey on Nov 17 2015, 7:54 PM.
Tags
None
Referenced Files
F14441749: D14501.diff
Thu, Dec 26, 7:13 AM
Unknown Object (File)
Mon, Dec 16, 4:23 PM
Unknown Object (File)
Wed, Dec 11, 12:58 AM
Unknown Object (File)
Thu, Dec 5, 11:56 PM
Unknown Object (File)
Wed, Dec 4, 7:51 PM
Unknown Object (File)
Sun, Dec 1, 10:07 AM
Unknown Object (File)
Sun, Dec 1, 12:19 AM
Unknown Object (File)
Sun, Dec 1, 12:18 AM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary
  • --version exists with 1 (At least for 1.5.1)
  • getLintCodeFromLinterConfigurationKey() was strangely used
Test Plan

run lint on some less code

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 8891
Build 10408: Run Core Tests
Build 10407: arc lint + arc unit

Event Timeline

avivey retitled this revision from to Update LESSC linter.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a subscriber: joshuaspence.

(This diff doesn't look very good; This is what git-show/git-diff looks like, and what I expected to see here:

pasted_file (1×693 px, 93 KB)

and this is what I'm seeing here (with "ignore most whitespace":

pasted_file (1×1 px, 193 KB)

)

You can see that diff by selecting "Whitespace Changes: Show All" (and see it exactly by then switching to unified mode):

Screen Shot 2015-11-17 at 11.58.54 AM.png (1×1 px, 311 KB)

Although it's a bit stripey, I think the whitespace-ignored diff is actually great -- it makes it very easy to see that you made exactly the expected changes. In the block version, it would be very hard to spot an accidental typo, like adding text in the middle of one of the strings or constants (say, a stray accidental keypress).

Ok, at least that's consistent with git.

Mostly I was surprised that the whole message-> block is marked red/green, when it just moved around (No whitespace changes in it; The striped area was un-indented).

epriestley added a reviewer: epriestley.

--version exists with 1 (At least for 1.5.1)

Maybe leave a comment about this? I think the behavior is a little unusual, and I could imagine some well-intentioned contributor removing this in the future if lessc 2 no longer exhibits the behavior.

Rest of this looks good, except for the local lessc failure, which we should probably figure out.

This revision now requires changes to proceed.Nov 18 2015, 4:26 PM
avivey edited edge metadata.

add comment about 1.5.1 exit code.

The unittest fail is, I think, because I was testing with 1.5.1 which is very old (2013-11), and just doesn't support
some features.
Running again with 2.5.3 (2015-09), the test passes.

Totally forgot about this one :)