Summary:
Ref T2039. Convert the `ArcanistPylintLinter` to an `ArcanistExternalLinter` and make it compatible with `.arclint`. In doing so, it was necessary to drop support of older versions of `pylint` by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an `ArcanistMissingLinterException` is thrown.
One caveat here is that support for `lint.pylint.codes.(error|warning|advice)` is dropped. Any installs that are relying on this configuration will need to migrate to using the `.arclint` file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.
Test Plan: Wrote and executed unit tests with `arc unit`. I ran the unit tests on all `pylint` releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9109
Summary: Use `__CLASS__` instead of hardcoding class names. Depends on D12804.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12805
Summary: Add a linter rule to advise against the use of hardcoded class names. Hardcoded class names make the code harder to refactor and it is generally preferable to use the `__CLASS__` magic constant instead.
Test Plan: This works, but needs some polish.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12605
Summary: Ref T8087. Prepares for eventually making these optional after T6030. See also T7443.
Test Plan:
- See the next change for the server-side part of this.
- With both patches applied, rigged the server to return `D123`.
- Created a revision, saw bare `D123`.
- Updated it with bare `D123`, things worked properly.
- Created this revision with full URIs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T8087
Differential Revision: https://secure.phabricator.com/D12748
Summary:
Ref T5955. This logic is a little cleaner than the previous version.
Don't require `~/.arcrc` to exist if the caller provides `--conduit-token`.
Test Plan:
- Made calls with `--conduit-token` and no `~/.arcrc`.
- Made calls with `--conduit-token` and a normal `~/.arcrc`.
- Made calls with normal `arc`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12750
Summary:
The ArcanistPhpunitTestResultParser has been changed
to use phutil_json_decode instead of json_decode in
rARCa4d33ef117aa8702181154b60ce1ce52bcfc119b
That has broken the functionality as json_decode has returned
an object before but phutil_json_decode does return an array.
The code has now been adopted to work with the array result
instead of an object.
Test Plan: Run a phpunit test case
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, epriestley
Differential Revision: https://secure.phabricator.com/D12751
Summary: Fixes T6311 and T5124 by returning all configured linters from `buildLinter()`, and making `ArcanistExternalLinter::checkBinaryConfiguration()` not crash if there's no executable to run.
Test Plan: `arc linters` in rP shows "Configured" and "ERROR" as appropriate; Adding a broken linter to `.arclint` in rARC doesn't invoke it's not actually needed, and prints error if it is.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6311
Differential Revision: https://secure.phabricator.com/D10773
Summary: Ref T5955. This makes it easier to write scripts which call Conduit via `arc call-conduit`.
Test Plan: Used `arc --conduit-token ... call-conduit user.whoami` to make calls as various users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12717
Summary: Support and prefer configuration from .arclint over Configuration Manager
Test Plan: arc lint with several combinations of values in .arcconfig and .arclint.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: vrusinov, jpoehls, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10704
Summary: Ref T7674. Remove support for passing data to linters via `STDIN`. This functionality exists primarily for pre-receive workflows (which don't have a working copy to act on), but these workflows are going away soon.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7674
Differential Revision: https://secure.phabricator.com/D12696
Summary: Ref T7215. This test case is consistently failing locally (with a `LIBXML_VERSION` of `20902`).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7215
Differential Revision: https://secure.phabricator.com/D12706
Summary: Remove the `ArcanistUncommittedChangesException` class which is unused after D11843.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12515
Summary: Move the `getSuperGlobalNames` method to `ArcanistBaseXHPASTLinter` so that it can be used within subclasses.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: virendra20888, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12473
Summary: Ref T7892. Memoize paths returned from `ARcanistLinter::getPaths()` to improve runtime performance.
Test Plan: Wiped ~0.5 seconds off the total time to lint rARC.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12520
Summary: Ref T7892. Improve the performance of `ArcanistXHPASTLinter::LINT_UNNECESSARY_SEMICOLON`.
Test Plan: Wiped 6 seconds off the total time to lint rARC.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12519
Summary: Ref T7892. Avoid reading the PHP compatibility information for every file being linted.
Test Plan:
```name=Before
real 1m24.327s
user 1m19.571s
sys 0m5.239s
```
```lang=After
real 1m12.029s
user 1m5.756s
sys 0m5.502s
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12518
Summary:
Fixes T5097. When linting a large list of paths (e.g., with `--everything`), do this internally:
$chunks = array_chunk($paths, 32);
foreach ($chunks as $chunk) {
$this->lintChunk($chunk);
}
This keeps the advantages of parallelism and artifact sharing for future-based linters, without having memory usage grow in an unbounded way.
These callbacks changed:
- `willLintPath()`: Useless, no meaningful implementations. Internalized the required side effect and broke the hook.
- `didRunLinters()`: Now useless, with no meaningful implementations. Broke the hook.
- `didLintPaths()`: New hook which executes opposite `willLintPaths()`.
- `lintPath()`: Linters no longer need to implement this method.
XHPAST now has an explicit way to release shared futures.
These minor changes also happened:
- Formalized the "linter ID", which is a semi-durable identifier for the cache.
- Removed linter -> exception explicit mapping, which was unused. We now just collect exceptions.
- We do the `canRun()` checks first (and separately) now.
- Share more service call profiling code.
- Fix an issue where the test harness would use the path on disk, even if configuration set a different path.
Test Plan:
- Ran `arc lint --everything` in `arcanist/`.
- With no chunking, saw **unstable** memory usage with a peak at 941 MB.
- With chunk size 32, saw **stable** memory usage with a peak at 269 MB.
- With chunk size 8, saw **stable** memory usage with a peak at 180 MB.
- Ran with `--trace` and saw profiling information.
- Created this diff.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5097
Differential Revision: https://secure.phabricator.com/D12501
Summary: The default Go Test Parser stopped showing the test function name at the end of the package after some Go upgrade. We fixed it locally and wanted to share it upstream.
Test Plan: We ran it on our test suite using go1.4.2 darwin/amd64. We have version-dependent code which cannot be compiled with earlier versions of Go, unfortunately.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T7845
Differential Revision: https://secure.phabricator.com/D12436
Summary: `pht`ize a bunch of strings in `ArcanistXHPASTLinter`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12392
Summary: Ref T7409. If `::` is surrounded by whitespace tokens and the whitespace token contains a newline character, allow it.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D12391
Summary: Although these don't do any harm, they show up in my editor which is configured to highlight trailing whitespace.
Test Plan: Submitted this diff... saw no trailing whitespace.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12369
Summary: Improve `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR` in handling multi-line arrays, see D12280 and D12281 for example. Depends on D12295.
Test Plan: Updated unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12296
Summary: Fixes T7464. JSHint is unable to exclude files from linting (via the `jshintignore` file) if the data is piped through `STDIN`. An alternative would be to pass `$options[] = '--filename='.$this->getActivePath()`, but `$this->getActivePath()` is not set when the command arguments are constructed.
Test Plan: Created a file and linted it with `ArcanistJSHintLinter`. Was able to exclude the file from linting with a `jshintignore` file.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7464
Differential Revision: https://secure.phabricator.com/D12298
Summary: The format of this file has changed. Depends on D12278.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12279
Summary:
Ref T7409. Adds a rule to detect unnecessary semicolons. The most common scenario I've seen in the wild is the use of semicolons after a class definition:
```lang=php
class MyClass {
// ...
};
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D12139
Summary: Ref T5655. This class was deprecated in D11673.
Test Plan: Wait a few months.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11740
Test Plan:
Add the following JSON to your `.arclint`:
```lang=json
"rubocop": {
"type": "rubocop",
"include": "(\\.rb$)"
}
```
Then, add a ruby file with errors, for instance:
```lang=ruby
def hello()
puts 'world'
end
```
Run `arc lint`. It should come up with something along the line of: "Omit the parentheses in defs when the method doesn't accept any arguments."
Reviewers: joshuaspence, remon, #blessed_reviewers, epriestley
Reviewed By: joshuaspence, #blessed_reviewers, epriestley
Subscribers: reu, calfzhou, jjooss, cburroughs, chad, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10738
Summary:
Fixes T7521. This separates things into two prompts and doesn't try to automatically add untracked files.
This also fixes T7512, or at least I can't reproduce it after this change.
Test Plan:
- Ran `arc diff` in a `git` directory with untracked files.
- Ran `arc diff` in a `svn` directory with untracked files.
- Ran `arc diff` in a `hg` directory with untracked files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7512, T7521
Differential Revision: https://secure.phabricator.com/D12258
Summary:
Ref T7594. Currently, if a chunk upload fails, we incorrectly swallow the failure and fall back to single-file upload, which will often fail by hitting size limits. This also silences the original error.
Instead, do chunk uploads outside the block so that any exceptions escape, and we don't try to fall back to single-file upload.
Mostly just trying to get more info about what's going wrong on @joshuaspence's install.
Test Plan: Faked an exception in chunk upload, ran `arc upload` on a big file, saw the exception displayed on the console.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, joshuaspence
Maniphest Tasks: T7594
Differential Revision: https://secure.phabricator.com/D12111
Summary: Ref T7149. Make sure we sit at "Resuming: 60%" or whatever while uploading the first chunk.
Test Plan: Ran `arc upload` on a large file, cancelled it, resumed it, got sensible progress bar.
Reviewers: chad, btrahan
Reviewed By: chad, btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12082
Summary: Ref T7149. This was just for testing and is no longer required.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12077
Summary: Ref T7149. This makes the client try to use the new `file.allocate` API before falling back to the old stuff.
Test Plan: Used `arc upload` to upload files. With chunking forced, uploaded chunked files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12061
Summary:
Refs D11990. When using `arc diff` with untracked files in the working
copy, add the untracked file(s) to the commit (as they weren't stashed or
ignored). Add the untracked paths to the list of changes in the editor
template, indicating that the files were added to the commit.
This doesn't add a separate prompt to add untracked files as per the
behaviour prior to D11843.
Test Plan:
Ran `arc diff` with only untracked files, answered yes to the 'create
new commit' prompt. Saw the commit-message with the updated changes
including untracked files. Completed the arc template, and got commit
containing uncommitted, unstaged and untracked files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11995
Summary: Fixes T7465. I think I just missed this when untangling the old logic.
Test Plan: Ran `arc diff` with //only// untrakced files, saw warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7465
Differential Revision: https://secure.phabricator.com/D11990
Summary:
We end up with one too few newline here in some workflows, like `arc land` with unstaged changes.
Root issue here is that `phutil_console_prompt|confirm` lead with too much whitespace but that's a harder fix.
Test Plan: Saw reasonable whitespace.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11927
Summary: This is supposed to just print out the base revision, but actually prints out the repository section first.
Test Plan: Ran `arc which`, `arc which --show-base`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11888
Summary:
Fixes T7344.
Currently, we use `phutil_console_prompt()`, which isn't a very good editor. Use the real $EDITOR instead.
100% of the logic here was also a gigantic mess. Clean it up.
Test Plan: Will update in a second with console output from this run.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7344
Differential Revision: https://secure.phabricator.com/D11843
Summary:
Ref T7152. Ref T1139.
- Tweak API.
- Move translations out of __init__ file.
Test Plan:
- Ran `arc`.
- Added a goofy translation and made sure it was working.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11746
Summary: After all that, I forgot to change this back.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11735
Summary:
pep8 has used both 2 (`1.2`) and 3 (`1.2.1`) digit versions. Losen
the version check to allow for both.
NOTE: This is the same regex as flake8.
Test Plan: `arc unit` with a 2 and 3 digit pep8 version on `$PATH`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D11728
Summary: Fixes T7046. Adds a linter rule to detect mismatched parameters for formatted strings. Originally I had considered putting this rule in `ArcanistPhutilXHPASTLinter`, but I later decided to move it to `ArcanistXHPASTLinter` as I think that it is general enough to be more widely useful.
Test Plan: This seems to work but needs some polish.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7046
Differential Revision: https://secure.phabricator.com/D11661
Summary: Ref T5655.
Test Plan: Ran `arc lint` with `lint.engine` set to `ComprehensiveLintEngine` and saw a deprecation notice.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11673
Summary: It should be safe to remove this now.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11714
Summary: I don't think that this provides too much value. I think that we should rework this to be inferred from the `.arcconfig` file perhaps?
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11662
Summary: Fixes T7113. This one was a bit trickier than others as the API output changed a bit. In particular, there is no "errors" emitted so much as the result set just doesn't include the answer if things are garbage. Ergo, check the "identifier map" to either check for diff existence or to lookup the phid to grab the actual diff data from the "data" part of the result.
Test Plan: called `arc backout D11665` and got some working output...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7113
Differential Revision: https://secure.phabricator.com/D11667
Summary: Fixes T7112. Nothing too difficult here.
Test Plan:
meta - submitting this with the new arcanist code
used conduit API to verify the difference between getdiff (just the latest diff) and querydiffs (all diffs that match, with the latest diff first in the set)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7112
Differential Revision: https://secure.phabricator.com/D11665
Summary:
Some commands (like `get-config`) do not require a working copy at all. Recent changes prevented these commands from running outside a VCS working copy.
Let `null` mean "no working copy".
See also <https://github.com/phacility/phabricator/issues/800>.
Test Plan:
- Ran `arc get-config` from outside a working copy.
- Ran `arc list` from outside a working copy, got an error.
- Ran `arc commit` in a Git working copy, got an error.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11643
Summary: Explicitly declare that the `arc branch` command is only supported under `git`.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11552
Summary: This workflow only works on git + mercurial.
Test Plan: I don't even subversion.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11626
Summary: This workflow does not work on subversion.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11625
Summary: It doesn't make any sense to run this command on another VCS.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11624
Summary: It doesn't make any sense to run this command on any other VCS.
Test Plan: Ran `arc svn-hook-pre-commit` and hit the usage exception.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11623
Summary: There is no need to check if the XHPAST binary is available here because this linter does not actually parse PHP, it only considers the library map.
Test Plan: Removed the XHPAST binary and ran `arc lint` on a bunch of files.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11528
Summary: Use `PhutilXHPASTBinary` methods instead of `xhpast_parse` functions. Depends on D11517.
Test Plan: N/A, this is a direct swap.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11613
Summary: Rely on the output of `xhpast --version` when determing the lint cache key.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11611
Summary: Instead of having an `ArcanistWorkflow` subclass explicitly throw an exception when run in an unsupported VCS, consolidate this code and move it to `arcanist.php`. In doing so, we lose some specificity in some of the error messages, but this otherwise feels cleaner. We could consider adding a `getUnsupportedRevisionControlSystemMessage()` method to provide a more tailored error message. Depends on D11604.
Test Plan:
Ran `arc bookmark` in a `git` working copy:
```
Usage Exception: `arc bookmark` is only supported under hg.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11550
Summary: Using `array('any')` to represent `array('git', 'hg', 'svn')` is a bit magical and leads to a lot of special-casing.
Test Plan: Verified that tab completion (ala `ArcanistShellCompleteWorkflow`) still worked.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11604
Summary: These tests are failing with the latest version of `jscs` (v1.10.0).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11520
Summary: A bunch of unit tests are failing with the latest version of `lessc` (v2.3.0). I decided to delete a bunch of test cases for this linter as we have far too many at the moment.
Test Plan: `arc unit`
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11521
Summary: Fixes T2461. Similarly to `arc lint --everything`, `arc unit --everything` should work without a base commit.
Test Plan: Removed the `.git/arc/default-relative-commit` file and ran `arc unit --everything`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T2461
Differential Revision: https://secure.phabricator.com/D11459
Summary: Allow `--severity=warning` to mean the same as `--severity warning`. Longer term, we should convert this code to use `PhutilArgumentParser`, although it doesn't seem that `PhutilArgumentParser` support British spelling ;)
Test Plan: Ran `arc lint --severity=warning`. Also `var_dump`ed `$args` to make sure it looked reasonable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11464
Summary: This requires PHP 5.4. Use `newv()` instead. I don't think the "without constructor" part is meaningful (or desirable). Simplify slightly.
Test Plan: `arc unit --everything`
Reviewers: hach-que, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11450
Summary: With a special guest appearance from `ArcanistGeneratedLinter`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11345
Summary: Fixes T5639. Allows the detection of undefined magic symbols, such as the use of `__DIR__` in a codebase that targets < PHP 5.3.0.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T5639
Differential Revision: https://secure.phabricator.com/D11386
Summary: Fixes T6830. Don't require `n_STATEMENT` nested under `n_DECLARE` to be written using braces.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6830
Differential Revision: https://secure.phabricator.com/D11350
Summary: The expectation is that this linter only raises errors.
Test Plan: This is mostly theoretical.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11331
Summary: Using `--reporter=raw` exposes raw JSON output rather than a limited XML output. This allows us to pull more context from the `coffeelint` output.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11320
Summary: It is more common for linters to exit with a non-zero status than it is for linters to return with a zero exit status, Really this function serves very little purposes, it simply determines whether or not to throw an exception if a non-zero status is returned by the external linter.
Test Plan: `arc unit`
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11322
Summary: Currently there is an issue when renderering linter messages with `ArcanistConsoleLintRenderer` if `$message->getChar()` returns `0` (i.e. the value is unset).
Test Plan:
**Before**
{F265586}
**After**
{F265587}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11319
Summary: `false` is the default return value for this method.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11314
Summary: `csslint` returns an `evidence` field which contains the line of the "original text" which produced the linter message (see 5dd84b259b/src/core/Reporter.js (L64)). We can use this data instead of trying to achieve the same result using `$this->getData($path)`.
Test Plan: {F265449}
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11311
Summary: Similar to D11198. This flag doesn't do anything when combined with `--format=lint-xml`.
Test Plan:
```lang=bash
> csslint *.css
csslint *.css
csslint: There are 1 problems in /home/joshua/workspace/github.com/phacility/arcanist/fail.css.
fail.css
1: error at line 1, col 1
Unexpected token '~' at line 1, col 1.
~
csslint: No errors in /home/joshua/workspace/github.com/phacility/arcanist/pass.css.
> csslint --quiet *.css
csslint --quiet *.css
csslint: There are 1 problems in /home/joshua/workspace/github.com/phacility/arcanist/fail.css.
fail.css
1: error at line 1, col 1
Unexpected token '~' at line 1, col 1.
~
> csslint --format=lint-xml *.css
<?xml version="1.0" encoding="utf-8"?><lint>
<file name="/home/joshua/workspace/github.com/phacility/arcanist/fail.css"><issue line="1" char="1" severity="error" reason="Unexpected token '~' at line 1, col 1." evidence="~"/></file>
</lint>
> csslint --format=lint-xml --quiet *.css
<?xml version="1.0" encoding="utf-8"?><lint>
<file name="/home/joshua/workspace/github.com/phacility/arcanist/fail.css"><issue line="1" char="1" severity="error" reason="Unexpected token '~' at line 1, col 1." evidence="~"/></file>
</lint>
```
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11308
Summary: The `src/lint/linter` directory is a bit cluttered at the moment.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11309
Summary: This seems to be off, at least for `puppet-lint` versions greater than 1.0.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11312
Summary: This test case is currently failing with various different versions of `puppet-lint`. I was considering fixing this test case, but I feel that we have more than enough test cases for `ArcanistPuppetLintLinter` anyway. Since this is an `ArcanistExternalLinter`, we only really need to test that the bindings are working as expected (rather than the linter functionality).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11313
Summary: Without explicitly passing `--error-level=all` to `puppet-lint`, the configuration file for `puppet-lint` could contain `--error-level=error`. This would limit the ability of `arc lint` to detect linter issues and therefore we should override this flag from the command line explicitly.
Test Plan: This is mostly theoretical.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11316
Summary: There's no need for these properties to be (implicitly) public.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11276
Summary: We can use `php --run "echo phpversion();"` to return a nice, compact representation of the PHP version.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11264
Summary: This directory was missed, but should have been moved in D11202.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11262
Summary: I'm not sure whether this is in any coding standards, but it seems to be an unspoken rule that methods should have a visiblity modifier (`public`, `protected` or `private`) explicitly specified. According to the [[http://php.net/manual/en/language.oop5.visibility.php#language.oop5.visiblity-methods | PHP documentation]], methods declared without any explicit visibility keyword are defined as public.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10687
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `ArcanistExternalLinter` and `ArcanistLinter` subclasses.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11237
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `ArcanistShellCompleteWorkflow` class.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11238
Summary: Ref T6822. This method needs to be public so that it can be called from `ArcanistLintersWorkflow`.
Test Plan: Ran `arc linters`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11239
Summary: I feel that we are abusing `ArcanistUsageException`. Throw a more tailed exception instead. Depends on D11197.
Test Plan: `arc lint`, I guess.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: avivey, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11205
Summary: D7952 added namespaces to `ArcanistUnitTestResult` but these are not exported and thus don't get sent from client to server.
Test Plan: Uploaded a diff and inspected the contents of the `phabricator_differential.differential_diffproperty` table.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11208
Summary: Ref T5655. `PHPUnitTestEngineTestCase` is the test case for `PhpunitTestEngine`.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11203
