Page MenuHomePhabricator

Add a LESS linter.
ClosedPublic

Authored by joshuaspence on May 6 2014, 4:01 AM.
Tags
None
Referenced Files
F14029171: D8992.id21352.diff
Fri, Nov 8, 7:56 PM
F14026868: D8992.diff
Fri, Nov 8, 2:46 AM
F13980135: D8992.id.diff
Sat, Oct 19, 8:38 AM
F13979893: D8992.diff
Sat, Oct 19, 7:04 AM
F13971957: D8992.diff
Thu, Oct 17, 4:21 PM
F13971926: D8992.id.diff
Thu, Oct 17, 4:11 PM
F13971924: D8992.id21352.diff
Thu, Oct 17, 4:10 PM
F13971906: D8992.id21355.diff
Thu, Oct 17, 4:01 PM
Subscribers

Details

Summary

This class provides an adapter for lessc.

Test Plan

Wrote and executed unit tests.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Add a LESS linter..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

One issue, maybe? Is getLinterConfigurationOptions() not actually required to use thsoe options in .arclint? I'd expect an exception if you try to specify them without overriding it.

src/lint/linter/ArcanistLesscLinter.php
7–8

We just render these blocks with remarkup:

[[ https://blahblah | lessc ]]
80–81

I would expect that you would need to implement getLinterConfigurationOptions() to support these?

Maybe we should prefix them too (lessc.strict-math).

I don't quite understand your comment. How are options passed from .arclint to the linters? I'm not sure myself.

joshuaspence edited edge metadata.

Fix commenting style.

I don't quite understand your comment. How are options passed from .arclint to the linters? I'm not sure myself.

ArcanistConfigurationDrivenLintEngine loads the configuration engine near the top of buildLinters(), near line 27:

It does a top-level typecheck for the file format (basically, it should contain a map of linter specifications):

PhutilTypeSpec::checkMap(
  $config,
  array(
    'linters' => 'map<string, map<string, wild>>',
    'debug'   => 'optional bool',
  ));

Then it goes through linters and typechecks each one of those, by calling getLinterConfigurationOptions(). This is implemented by:

  • ArcanistLinter (adds severity, severity.rules)
  • ArcanistExternalLinter (adds bin, flags, interpreter)
  • optionally the concrete linter class itself, to add more flags

Then the actual typecheck happens, near line 57, which checks type, include and exclude:

PhutilTypeSpec::checkMap(
  $spec,
  array(
    'type' => 'string',
    'include' => 'optional string | list<string>',
    'exclude' => 'optional string | list<string>',
  ) + $more);

So the expectation is that if you have a .arclint file and add arbitrary configuration to a linter section, you'll get an exception when you run lint if you didn't actually specify that the config keys are known (some day, this should be a little prettier):

$ arc lint --rev HEAD^
Exception
Got unexpected parameters: quack
(Run with --trace for a full exception trace.)

So to add options strict-math and strict-units, I'd expect you to need something like (I namespaced the option names here):

public function getLinterConfigurationOptions() {
  return parent::getLinterConfigurationOptions() + array(
    'lessc.strict-math' => 'optional bool',
    'lessc.strict-units' => 'optional bool',
  );
}

...otherwise, you'll get that exception if you specify these in .arclint and run lint.

  • Provide an implementation for getLinterConfigurationOptions.
  • Prefix configuration keys with lessc..
This revision is now accepted and ready to land.May 6 2014, 1:50 PM
epriestley updated this revision to Diff 21362.

Closed by commit rARC88ff113f921c (authored by @joshuaspence, committed by @epriestley).