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
Unknown Object (File)
Thu, Apr 11, 4:21 AM
Unknown Object (File)
Mon, Apr 8, 2:41 PM
Unknown Object (File)
Sun, Apr 7, 5:25 PM
Unknown Object (File)
Sat, Apr 6, 7:41 AM
Unknown Object (File)
Sat, Mar 30, 12:27 AM
Unknown Object (File)
Sat, Mar 23, 12:51 AM
Unknown Object (File)
Sat, Mar 23, 12:50 AM
Unknown Object (File)
Sat, Mar 23, 12:50 AM
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