Add a `getVersion` function to `ArcanistExternalLinter`.
ClosedPublic
Actions

Authored by joshuaspence on May 4 2014, 11:47 AM.

Details

Reviewers

epriestley
chad

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4954: Allow version dependencies to be specified for external linters

Commits

rARCf2b341ae038a: Add a `getVersion` function to `ArcanistExternalLinter`.

Summary

This method will, theoretically, allow arc lint to be configured to require some minimum version of an external linter (although this would probably require significantly more work).

Additionally, the existence of this method simplifies the getCacheVersion function which, previously, was implemented by the external linters individually. Instead, a general approach to determining the version for cacheing purposes can be used.

Fixes T4954.

Test Plan

I'm not sure how to test this.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

joshuaspence updated this revision to Diff 21278.May 4 2014, 11:47 AM

joshuaspence retitled this revision from to Add a `getVersion` function to `ArcanistExternalLinter`..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a task: T4954: Allow version dependencies to be specified for external linters.

Herald added reviewers: Blessed Reviewers, chad. · View Herald TranscriptMay 4 2014, 11:47 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B228: Diff 21278.May 4 2014, 11:48 AM

The flake8 issue might be a version difference -- what version do you have? The tests pass here and I'm on:

2.0 (pep8: 1.4.6, pyflakes: 0.7.3, mccabe: 0.2.1)

If your version is newer, we should probably update the test to the modern version of flake8 and then skip it for versions older than whatever you have (once this patch lets us do that). That can happen elsewhere, though (and must wait for this patch anyway).

For this patch:

I think we should reduce the strictness slightly: provide a default implementation that returns null, and not throw from getCacheVersion(). For example, we have a subclass in phabricator/ which would theoretically break without this (it currently extends ArcanistLinter not ArcanistExternalLinter, so it wouldn't actually break, but that's just because it hasn't been updated) relatively needlessly, and which doesn't really have a meaningful version string. I think it's OK for some linters to not implement this method. We could throw a little later on, if the user tried to configure a minimum version for such a linter: at that point we can motivate things better.
Some versions of PHP and/or PCRE require the (?P<name>...) form, not the shorter (?<name>...) form. We originally used the shorter form and saw some issues, so these patterns should use the explicit P form.
Could the \d.\d.\d patterns be looser? We can wait until we hit an actual issue, but if we eventually get a 2.0.0dev or something maybe we should be more permissive. I think version_compare() handles most of these cases sensibly.

This revision now requires changes to proceed.May 5 2014, 2:44 PM

I think we should reduce the strictness slightly: provide a default implementation that returns null, and not throw from getCacheVersion().

The throwing of an exception from getCacheVersion was intended to throw if getVersion returns false, indicating that it couldn't pass the version number from the output (although I checked for falsey-ness rather than null).

Right, my suggestion is that if version information isn't available we just do our best in getCacheVersion() (e.g., returning flags only). Basically, this implementation makes getVersion() required, and I think it should be optional by default instead (e.g., provide an empty implementation, don't throw when failing to determine versions) and only required if the configuration attempts to make use of the version number (e.g., by setting a minimum version).

Yeah OK. I suppose that even if the version number fails to parse, it is reasonable to avoid throwing an exception unless a minimum version has been requested (when this functionality exists).

In the interest of retain compatibility, I should probably remove the final declaration from getCacheVersion.

I did consider writing a general solution to parsing the version number, which could live in ArcanistExternalLinter::getVersion. However, I suspected that this would be too error prone (although, I didn't actually verify this).

Could the \d.\d.\d patterns be looser? We can wait until we hit an actual issue, but if we eventually get a 2.0.0dev or something maybe we should be more permissive. I think version_compare() handles most of these cases sensibly.

Probably, but I'd be hesitant to do this without having some real data (output from various versions of the external linter when executed with the --version flag). For example, would I be expecting vX.Y.Z-dev or vX.Y.Zdev?