Page MenuHomePhabricator

Make it easier to parse external linter versions
Needs ReviewPublic

Authored by joshuaspence on May 20 2019, 1:57 AM.
Tags
None
Referenced Files
F14016788: D20526.id49030.diff
Mon, Nov 4, 10:43 AM
F14016787: D20526.id48989.diff
Mon, Nov 4, 10:43 AM
F14016786: D20526.id48985.diff
Mon, Nov 4, 10:43 AM
F14016785: D20526.id48953.diff
Mon, Nov 4, 10:43 AM
F14016784: D20526.id48952.diff
Mon, Nov 4, 10:43 AM
F14016783: D20526.id.diff
Mon, Nov 4, 10:43 AM
F14014325: D20526.diff
Sun, Nov 3, 1:57 AM
F13995810: D20526.id48952.diff
Wed, Oct 23, 3:46 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Currently pretty much every external linter has copy-pasted the following code (with minor variations):

public function getVersion() {
  list($stdout) = execx('%C --version', $this->getExecutableCommand());

  $matches = null;
  if (!preg_match('/^v(?P<version>\d+\.\d+\.\d+)$/', $stdout, $matches)) {
    return null;
  }

  return $matches['version'];
}

We can reduce the boilerplate code here by providing a simpler interface consisting of getVersionFlags and parseVersionOutput, similar to getMandatoryFlags and parseLinterOutput. This change also makes it possible for me to proxy the getVersion method in [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php | ArcanistDockerContainerLinterProxy]].

Test Plan

Installed a bunch of the external linters and ran unit tests.

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/lint/linter/ArcanistExternalLinter.php:383XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 22828
Build 31311: Run Core Tests
Build 31310: arc lint + arc unit

Event Timeline

src/lint/linter/ArcanistExternalLinter.php
401

I'm open to suggestions for a better name for this method.

408

I'm open to suggestions for a better name for this method.

Semi-related thought: the current behavior of getVersion (return a string if successful, false if unable to determine the version or null if no version exists) is rather awkward and I feel like we should throw some sort of ArcanistUnableToDetermineLinterVersionException instead of returning false.

Implement parseVersionOutput in ArcanistExternalLinter so that it can be called from within another instance of ArcanistExternalLinter