Summary:
Depends on D9906.
This adds `arc start`, `arc stop` and `arc tracking` for tracking tasks, diffs and other objects in Phrequent.
Test Plan: Tested this against a local install.
Reviewers: skyronic, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: maxhodak, hach-que, aran, epriestley, Korvin
Maniphest Tasks: T3569, T3969
Differential Revision: https://secure.phabricator.com/D7327
Summary:
Fixes T5555. Normally, when we `svn diff subdir/`, we use `--depth empty` to get only changes for the directory itself (usually, property changes).
However, this flag has no effect if the directory is newly added.
Adjust the diff parser so that if two sets of hunks are specified for a single file in a raw diff, we let the last one win instead of including both. This approach is a broadly more reasonable interpretation of these diffs.
Test Plan:
- Added a new file in a new subdirectory in Subversion.
- Ran `arc diff --only`.
- No double file content in resulting diff.
- Added unit test.
- There's fairly comprehensive unit test coverage for this stuff.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5555
Differential Revision: https://secure.phabricator.com/D9921
Summary:
These are documented as being identical, but `git diff a b` works if `a` is a tree (for example, `4b825d...`, the empty tree hash), but `git diff a..b` does not.
Particularly, with the `a..b` form, `arc diff --base arc:empty` does not work. With the `a b` form, it does.
Test Plan: Ran `arc diff --base arc:empty` in a repository and got a diff.
Reviewers: btrahan, talshiri
Reviewed By: talshiri
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9898
Summary: The `ArcanistXHPASTLinter` is no longer //too// specific to Phabricator (Phabricator-specific XHPAST lints generally live in `ArcanistPhutilXHPASTLinter`) and //should// be better suited for general purpose usage.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9892
Summary: Change `==` to `===`. This is a little bit cleaner because we shouldn't need type coercion here.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9893
Summary: We manually quote this in a couple of places. That works fine on Lunix, but does not work on Windows. Instead, explicitly parameterize the command so the correct quoting rules are applied for the OS.
Test Plan: See IRC; windows user had issues fixed by this. `arc:upstream` still works locally.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9868
Summary: Fixes T4418. Allows Maniphests created through the `arc todo` workflow to have projects assigned.
Test Plan:
```
> ./bin/arc --trace --conduit-uri='http://phabricator.joshuaspence.com' todo "Test project" --project foo --project bar
libphutil loaded from '/home/joshua/workspace/github.com/phacility/libphutil/src'.
arcanist loaded from '/home/joshua/workspace/github.com/phacility/arcanist/src'.
Config: Reading user configuration file "/home/joshua/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/home/joshua/workspace/github.com/phacility/arcanist/.arcconfig".
Working Copy: Path "/home/joshua/workspace/github.com/phacility/arcanist" is part of `git` working copy "/home/joshua/workspace/github.com/phacility/arcanist".
Working Copy: Project root is at "/home/joshua/workspace/github.com/phacility/arcanist".
Config: Did not find local configuration at "/home/joshua/workspace/github.com/phacility/arcanist/.git/arc/config".
Loading phutil library from '/home/joshua/workspace/github.com/phacility/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 618>
>>> [1] <http> http://phabricator.joshuaspence.com/api/conduit.connect
<<< [1] <http> 1,050,487 us
<<< [0] <conduit> 1,051,585 us
>>> [2] <conduit> project.query() <bytes = 199>
>>> [3] <http> http://phabricator.joshuaspence.com/api/project.query
<<< [3] <http> 294,584 us
<<< [2] <conduit> 294,986 us
>>> [4] <conduit> maniphest.createtask() <bytes = 313>
>>> [5] <http> http://phabricator.joshuaspence.com/api/maniphest.createtask
<<< [5] <http> 637,693 us
<<< [4] <conduit> 638,098 us
Created task T6: 'Test project' at http://phabricator.joshuaspence.com/T6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4418
Differential Revision: https://secure.phabricator.com/D9457
Summary: Fixes T5577. If the `ArcanistPhutilLibraryLinter` lints a PHP file that contains a syntax error, it will die horribly. Instead, force it to continue as if nothing was wrong.
Test Plan: Introduced a PHP syntax error. Ran `arc lint` and made sure the output looked reasonable.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5577
Differential Revision: https://secure.phabricator.com/D9865
Summary: Ref T5577. Decrease the priority of the `ArcanistPhutilLibraryLinter` such that it is lower than that of the `ArcanistXHPASTLinter`. There isn't any specific reason that we should do this, although it seems to generally be a reasonable idea.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5577
Differential Revision: https://secure.phabricator.com/D9866
Summary: Ref T5577. Modify `ArcanistPhutilLibraryLinter` to use `PhutilLibraryMapBuilder` instead of using an `ExecFuture` to run `phutil_rebuild_map.php`.
Test Plan: Ran `arc lint`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5577
Differential Revision: https://secure.phabricator.com/D9864
Summary: Update `ArcanistInfrastructureTestCase` after D9860 and D9861.
Test Plan: `arc unit` should do the trick.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9863
Summary: I'm pretty sure that `@group` annotations are useless now... I believe that they were originally used by Diviner?
Test Plan: Eye-balled it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9855
Summary: After D9576, `LINT_PHP_53_FEATURES` and `LINT_PHP_54_FEATURES` are deprecated. They have been replaced by `LINT_PHP_COMPATIBILITY`.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9657
Summary: Modify `ArcanistInfrastructureTestCase::testLibraryMap` to use `phutil_get_current_library_name()` instead of hard-coding the library name.
Test Plan: See D9844.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9846
Summary:
Improve the `ArcanistInfrastructureTestCase` unit tests such that they will fail if any of the following conditions are satisfied:
- A symbol referenced in the `__phutil_library_map__.php` file no longer exists.
- A symbol exists in the library but is not referenced within the `__phutil_library_map__.php` file.
- A symbol extends from a different parent symbol than that declared in the `__phutil_library_map.php` file.
Test Plan: See D9824
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9826
Summary: Currently, the `ArcanistSpellingLinter` loads data from `ArcanistSpellingDefaultData`, with no way to configure the linter from an `.arclint` file. Instead we should define a format for a "dictionary" file, of which the `ArvcanistSpellingLinter` can load and of which the paths are easily configured through `.arclint`.
Test Plan:
Updated the test case and ran `arc unit`.
NOTE: I have removed the `LINT_SPELLING_PICKY` and `LINT_SPELLING_IMPORTANT` constants and replaced them with `LINT_SPELLING_FULL` and `LINT_SPELLING_PARTIAL`. This was done because it simplifies the implementation considerably and makes customization of the `ArcanistSpellingLinter` simpler, but also because these constants were not widely used in the existing implementation.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9805
Summary: There were two callsites which needed some information from the username. Only one worked "correctly", causing `arc diff` to not amend commits anymore because the author could not be parser.
Test Plan: run `arc diff` with changes
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9797
Summary:
https://github.com/mdevils/node-jscs/issues/444 has been resolved in commit [[0de667bac2 | 0de667bac2491ba04d930ff94e990965213ba36b]], which means that `jscs` can be easily run on files with any extension.
NOTE: JSCS version 1.5.7 is required.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9789
Summary: Fixes T5466. An image is an example of a binary which should //not// be executable. Modify the `ArcanistChmodLinter` to disallow certain blacklisted MIME types from being executable.
Test Plan: Created an executable image file and ran `arc lint` over this file.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: richardvanvelzen, epriestley, Korvin
Maniphest Tasks: T5466
Differential Revision: https://secure.phabricator.com/D9723
Summary: See D9681#22.
Test Plan: Created an empty `.git/arc/config` file... no exception was thrown.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9704
Summary:
Throw a useful error message when an `.arcconfig` file is not valid JSON.
Depends on D9697.
Test Plan:
Modified an `.arcconfig` file to be invalid JSON.
```
> arc lint
Usage Exception: Your '~/.arcrc' file is not a valid JSON file.
Parse error on line 18 at column 4: Expected: 'STRING' - It appears you have an extra trailing comma
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9681
Summary: Throw a useful error message when an `.arclint` file is not valid JSON.
Test Plan:
Modified an `.arclint` file to be invalid JSON.
```
> arc lint
Exception
Parse error on line 24 at column 5: Expected one of: 'EOF', '}', ',', ']'
(Run with --trace for a full exception trace.)
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9679
Summary: If JSHint encounters //too many// errors (by default, more than 50) errors, then it quits prematurely and raises an additional (rather unhelpful) "Too Many Errors" error. We should just ignore this additional error.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9693
Summary:
The error context (a.k.a. "original text") as provided by `jshint` is not very useful and actually causes `arc lint` to display the lint message incorrectly.
{F169277}
The underlying problem here is that the error context from `jshint` contains the entire line from the input file rather than just the offending source code.
Test Plan: Ran `arc lint -- webroot/rsrc/js/core/behavior-hovercard.js` (in rP) and verified that the output looked reasonable.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9675
Summary: Fixes T5442. `arc lint --everything` currently uses a `FileFinder` to discover paths for linting. A consequence of this is that files that are ignored or otherwise excluded from version control are linted.
Test Plan: Ran `arc lint --everything` in rPHU and noticed that I wasn't prompted to add a trailing newline to `src/.phutil_module_cache`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5442
Differential Revision: https://secure.phabricator.com/D9674
Summary: As discussed in D9097, the `ArcanistScalaSBTLinter` isn't //really// a linter. Eventually we should add support for a proper Scala linter, but I think that supporting the `ArcanistScalaSBTLinter` as-is is more effort than it is worth.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9664
Summary: This `LINT_PHP_53_FEATURES` should have been replaced with `LINT_PHP_COMPATIBILITY` in D9576.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9655
Summary: See inline comments on D9576. Also replace `json_decode` with `phutil_json_decode`, for better error handling.
Test Plan: Make sure `arc unit` still works.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9656
Summary:
Ref T5297. Utilize `PhutilJSONParser` to create a native JSON linter (native in that it is pure PHP with no external dependencies).
Since [[https://github.com/Seldaek/jsonlint | JsonLint]] is literally a PHP port of [[https://github.com/zaach/jsonlint | jsonlint]], I figured that we should also deprecate `ArcanistJSONLintLinter`. `ArcanistJSONLinter` //should// behave identically to `ArcanistJSONLintLinter`.
Depends on D9634.
Test Plan: Ran the `ArcanistJSONLintLinter` unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5297
Differential Revision: https://secure.phabricator.com/D9628
Summary: This was broken in D9641. If `$config` is not set, then a `PhutilJSONParserException` will be thrown. The expected behaviour is that it is okay to not explicitly set any configuration.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9643
Summary: Depends on D9634. `phutil_json_decode` now throws an exception on invalid JSON.
Test Plan: Ran the unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9641
Summary: Various tidying up of linting code.
Test Plan: `arc lint` and `arc unit` still pass.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9625
Summary: Self explanatory. Also fixed the character offset to start at 1 instead of 0 (because the other linters seem to do this).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9626
Summary:
In D9595, we stopped parsing short-form "Differential Revision:" fields in commit messages, and only accept URLs.
I have one of the older style commit messages in my local `arcanist/`, so now we go down this parse failure branch in `arc branch`. This has never worked quite correctly, and if the parse fails we end up with a bad branch dictionary that is missing fields.
Test Plan: Ran `arc branch` in a working copy with an old `Differential Revision:` field at the head of a branch. Before patch: explosions; after patch: works great.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9620
Summary:
There's some sort of race inside `git` here, where the `git diff-files` command exits with different results some of the time when run in parallel with `git ls-files` or `git diff` (running either command was sufficient to trigger the race).
Run it separately to avoid the race.
I poked around the `git` source a little bit but quickly lost interest given that the issue seems fixed and this workaround is essentially reasonable.
Test Plan: Ran test 20x in a row without failures.
Reviewers: hach-que
Reviewed By: hach-que
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9616
Summary: The output from `arc help --full` is missing a newline character.
Test Plan:
**Before**
```
> arc help --full
--skip-arcconfig
Skip the working copy configuration file
--arcrc-file filename
Use provided file instead of ~/.arcrc.>
```
**After**
```
> arc help --full
--skip-arcconfig
Skip the working copy configuration file
--arcrc-file filename
Use provided file instead of ~/.arcrc.
>
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9608
Summary:
This leads to information being lost when others do `arc patch` because the name is used as the email address.
For example:
username = Richard van Velzen
Would give:
'authorName' => null,
'authorEmail' => 'Richard van Velzen'
Test Plan:
ran it through my head a couple of times, and tested it with the common options which all gave the expected result:
'rvanvelzen@company.com',
'Richard van Velzen',
'Richard van Velzen <rvanvelzen@company.com>',
'Richard van Velzen rvanvelzen@company.com',
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9605
Summary: `PhutilConsoleTable` does a better job at aligning columns. We still probably need to do some work to `PhutilConsoleTable` to set a maximum width for a specified column, or elect which columns can be truncated etc but this can probably come later.
Test Plan: {F167687}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9601
Summary: This has been a `TODO` for a few years now... no-one should be using the old-school (non-URL) syntax anymore.
Test Plan: I'm actually not sure if it's okay to drop support for this... please verify.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9595
Summary: Fixes T5385. Provide a flexible means of setting a minimum PHP version for the `ArcanistXHPASTLinter`, instead of relying on `ArcanistXHPASTLinter::LINT_PHP_53_FEATURES` and `ArcanistXHPASTLinter::LINT_PHP_54_FEATURES`.
Test Plan: Fixed up and ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5385
Differential Revision: https://secure.phabricator.com/D9576
Summary: This linter rule is very specific to the Phabricator and it is unlikely that it is being used elsewhere. We should move it to `ArcanistPhutilXHPASTLinter`, where other Phabricator-specific linting code lives.
Test Plan: Moved the corresponding test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9590
Summary:
This is useful for wrapper scripts that want to customize arcanist's behavior without affecting the global configuration.
This can be implemented with arcanist_configuration entry in .arcconfig, however it is currently limited to
per-project settings, and this feature makes writing wrapper scripts a little easier.
Test Plan: arc diff --set-config editor=vim (yeah yeah, crappy test case)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9442
Summary: Add a linter which wraps around [[https://github.com/mdevils/node-jscs | JSCS]].
Test Plan: Currently, we can't test this linter with out existing infrastructure. Specifically, `jscs` will only lint `*.js` files. I have done a fair bit of manual testing though.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9565
Summary: This method has been deprecated for a while now (see rARC0d212ccf5a7ac5fc75d2d3036cb3a0bee220fd02). It should be safe to remove.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9466
Summary: I can't find an analogous flag for hg.
Test Plan: I don't have any precommit hooks set up on my machine so I can't test this easily, but I think this seems pretty harmless?
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9523
Summary: Allows the `ArcanistXHPASTLinter` to determine whether constants can be used, based on the target PHP version. Also added class methods to the compatibility information, although this isn't used yet (it is small anyway).
Test Plan: Created a test file that contained the `JSON_PRETTY_PRINT` constant. Verified that a linter error was raised.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9571
Summary:
Fixes T5377. The current `scripts/update_compat_info.php` script works for PHP CompatInfo version 2, but doesn't work with the newer version 3.
There are a few breaking changes in version 3 that had to be addressed:
- PHP 5.3 is required. Whilst Arcanist is generally compatible with PHP 5.2, I don't think that having this dependency presents any real issues because it is purely a development tool that is rarely updated.
- [[https://getcomposer.org/ | Composer]] is used for packaging, which makes including the library slightly more complicated. Basically, I had to install `PHP_CompatInfo` globally with Composer (`composer global require "bartlett/php-compatinfo"` and then symlink `~/.composer/vendor` into `externals/includes`.
Test Plan: Compared the `resources/php_compat_info.json` file. There are a bunch of functions/classes that //were// in this file but are no longer, but I think that I've covered the most popular extensions.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5377
Differential Revision: https://secure.phabricator.com/D9568
Summary: Just some minor tidying up. I don't think that these methods should ever be overridden in subclasses.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9569
Summary: See D9557. Add a unit test to ensure that `src/__phutil_library_map__.php` is up-to-date.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9558
Summary: As per the TODO comment, now that `.arclint` is more mature, we should be able to restore PEP8 errors.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9473
Summary:
This `ArcanistLesscLinter::getCacheVersion` should have been renamed to `getVersion` as of D8971, but wasn't updated.
Also fix the regex to properly capture version information.
Test Plan: It's pretty difficult to test this stuff.... I have an idea that I will submit a separate diff for.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9475
Summary:
Introducing `--head` caused us to run `git diff base..head` explicitly.
However, we can now hit this workflow:
- We resolve `HEAD` as commit `aaaa1`.
- This is cached.
- We notice dirty working copy changes and prompt the user to amend them to HEAD.
- The user accepts the amend.
- We amend, creating commit `bbbb2`.
- We dirty the commit range and reload the working copy. This //does not// dirty the cache of HEAD.
- We run `git diff`, but it uses the old cached HEAD: `git diff base..aaaa1`.
- This works fine (`aaaa1` still exists, it's just not on any branch) but produces the wrong diff (without amended changes).
To resolve this, implement the "dirty the cache when the range reloads" hook.
Also never try to amend if the user provides `--head`.
Test Plan:
Ran `arc diff --only --trace` in a working copy with a new commit and some uncommitted changes.
- Prior to this change, saw a `git diff base..aaaa1` command and the wrong diff.
- After this change, saw a `git diff base..bbbb2` command and the correct diff.
Reviewers: chad, csilvers, talshiri
Reviewed By: talshiri
Subscribers: epriestley, spicyj
Differential Revision: https://secure.phabricator.com/D9506
Summary:
This adds support for passing range of commits for arc diff. This is useful when you want to submit code reviews for past commits without mucking around with the working copy.
This will probably require changes :)
Test Plan: Tested locally, but totally need to add tests for this
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9369
Summary: This function has been deprecated for a long time (see rARC8150fdf044818c503a588c1b3ef0ddbb93cfa1be). It should be safe to remove it now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9465
Summary: Currently, class and interface names are converted to lowercase when comparing minimum and maximum versions. This is necessary in order to compare with the data from `php_compat_info.json` but causes the lint message to be slightly misleading.
Test Plan:
Linted a test file.
**Before**
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `iterator` was not introduced until
PHP 5.3.0.
1 <?php
2
>>> 3 final class FooBar implements Iterator {}
```
**After**
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `Iterator` was not introduced until
PHP 5.3.0.
1 <?php
2
>>> 3 final class FooBar implements Iterator {}
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9471
Summary: This workflow has been deprecated for a long time now. At some stage, it should be removed.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9463
Summary: Fixes T5300. Currently, if a dead symbolic link is linted, all kinds of errors will be thrown by most linters because they will try to read the (non-existent) file contents. Instead, let's not lint symbolic links by default. In the case that the target of a symbolic link is inside the working copy, then it should be being linted anyway.
Test Plan: Created a symbolic link and verified that it wasn't linted (by any linter other than the `ArcanistFilenameLinter`).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5300
Differential Revision: https://secure.phabricator.com/D9448
Summary: Applied various linter fixes. Also make the `.editorconfig` file a bit more specific. Unfortunately, `arc lint --apply-patches` currently modifies some test data that it shouldn't, but this should be fixed after T5105.
Test Plan: Ran `arc unit` to make sure things weren't broken.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9440
Summary:
Currently the template is single quoted, but Windows only supports double quotes. This meant that the output would be like:
lang=text
'aabbccddeeffaabbccddeeffaabbccddeeff0123
'
Which is clearly wrong.
This is displayed like that in Phabricator as well, which is confusing.
Test Plan: ran `arc diff` on a Windows machine and saw the correct behaviour.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9450
Summary: Currently, `'foo'. // Some comment` is not allowed by the `ArcanistXHPASTLinter::LINT_BINARY_EXPRESSION_SPACING` rule. I believe that in the case of a trailing comment, we //should// allow whitespace after the `.` operator.
Test Plan: Wrote and executed a unit test for this case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9438
Summary:
Currently, the `ArcanistSpellingDefaultData` is marked with a `@nolint` annotation. This means that it doesn't get linted by any linters. Really, the intention here was to make sure that this file isn't linted by the `ArcanistSpellingLinter` linter.
Now that the `.arclint` file is more mature, we can easily just exclude this file from being linted //only// by the spelling linter, whilst allowing other linters to run.
Test Plan: Ran `arc lint`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9432
Summary: Fixes T5298. This bug was probably introduced in D9248. It looks like I forgot to update some references to `$version`.
Test Plan: Ran `arc lint --everything` in rPHU.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5298
Differential Revision: https://secure.phabricator.com/D9434
Summary: Fixes T5209. It makes sense for the `ArcanistXHPASTLinter` to implement the `getVersion` method in order to be able to enforce version requirements (as in T4954).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5209
Differential Revision: https://secure.phabricator.com/D9317
Summary: We have coverage for normal binary operators, but not these unusual cases.
Test Plan: Added and executed unit tests.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9314
Summary: It seems reasonable to transform `arc --version` into `arc version`. The version command is similar to the help command in that it is a command that users unfamiliar with Arcanist will probably try to run at some stage.
Test Plan: Ran `arc --version`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9274
Summary: A `version` workflow would be useful, especially for less technical users. Additionally, whenever I am faced with a new command I reasonably expect `$CMD [--help|help]` and `$CMD [--version|version]` to work.
Test Plan: Ran `arc version`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9265
Summary: It appears to have never really work? At least as far as phabricator.uri (empirically).
Test Plan:
removed ~/.arcrc, run call-conduit user.whoami with --arcrc-file and --trace.
set-config, get-config and alias also read and write to the right place now.
Reviewers: avivey
Reviewed By: avivey
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9263
Summary: Ran `arc lint --apply-patches --everything` over rARC, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed //most// of the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9269
Summary:
This is all derped up, and we ignore the error so old installs don't break.
Underp it.
Test Plan: Did `arc land`, verified that the call actually went through successfully.
Reviewers: btrahan, davedash
Reviewed By: davedash
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9271
Summary: Create bindings for `golint` as an external linter.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9246
Summary: `$engine` will be undefined here. This was just copy/paste derp, combined with me accidentally having an `.arclint` file in a place I didn't expect to have one when I tried to test this.
Test Plan: Ran `arc linters` in a totally bare, non-.arclint-having working copy
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9255
Summary: Ref T5141. In order to be able to warn when deprecated functions are used, we need to be aware of from which version the functions were deprecated.
Test Plan: Ran `arc lint` and made sure no unexpected warnings were raised.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D9248
Summary: Ref T5141. Currently, `php_compat_info.json` is hardcoded to support PHP 5.2.3. Instead, store as much version information as possible in `php_compat_info.json` and filter accordingly in `ArcanistXHPASTLinter.`
Test Plan: Ran `arc lint` and made sure no additional warnings were raised.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D9247
Summary:
Currently, when code has a block like:
} catch (Exception $ex) {
...we attempt to mark "$ex" as declared. However, we incorrectly mark every variable used anywhere in the block as declared. This means we'll never raise this warning in a `catch` block.
Instead //only// mark the caught exception as declared.
Test Plan: Added a failing unit test and made it pass.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: lpriestley, epriestley
Differential Revision: https://secure.phabricator.com/D9239
Summary: Handles an issue in windows with large code bases not squash merging correctly, this only catches the issue it doesn't really do any recovery.
Test Plan: We have been running this change in our work environment, it's a hard bug to replicate but when it has reared it's head this has caught it.
Reviewers: waynea, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T4884
Differential Revision: https://secure.phabricator.com/D8729
Summary: Pyflakes doesn't have any message codes, so we can't do anything with severities.
Test Plan: Ran `arc lint` with severities on one of these linters, got an error.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9222
Summary:
Fixes T4952. Several issues:
- You review configuration values with `arc set-config --show`. This makes no sense and never has, I think it just predated `arc get-config` or was easier or something.
- Instead, review values with `arc get-config` and review details with `arc get-config --verbose`.
- Show better and more detailed information about all config sources.
- Establish and show default values from a new "default" source.
- With `--trace` include more information about attempts to read configuration files.
Test Plan:
Ran `arc get-config --trace --verbose` in various working directories and received sensible-looking output.
{F156247}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4952
Differential Revision: https://secure.phabricator.com/D9172
Summary: This string is quite silly!
Test Plan: `arc land --hold` in an affected working copy.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9212
Summary: Ref T2039. Now that the `ArcanistConfigurationDrivenLintEngine` has gained more widespread use, we do no longer expect users to need to write their own lint engine. The documentation has already been updated to reflect this.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9198
Summary: After D9057 and D9064 this class is no longer used.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9197
Summary: The XHPAST double quotes rule autofixes strings which use double quotes where single quotes would suffice. In the case in which the double-quoted string contained an escaped double quote, the autofix string can be improved by removing the escape symbol.
Test Plan: Modified existing unit tests to cover this case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9196
Summary: Fixes T3922. Add a linter which complains if files are executable when they shoudn't be.
Test Plan: Tested by modifying the `.arclint` file of the rARC repository and running `arc lint --everything`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3922
Differential Revision: https://secure.phabricator.com/D9187
Summary: To me, it seems that these methods should not be overridden by subclasses
Test Plan: `arc lint` and `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7960
Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7959
Conflicts:
src/workflow/ArcanistBaseWorkflow.php
Summary: Ref T2039. Convert the `ArcanistScalaSBTLinter` into an `ArcanistExternalLinter` and make it compatible with `.arclint`.
Test Plan: I can't really test this because I don't have any Scala projects and we don't have any test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: codeblock, epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9097
Summary: Previously, the `ArcanistXHPASTLinter` was suggesting that `"\1"` be written as `'\1'`, which is incorrect. Therefore, I ensured that all of the cases listed in the [[http://www.php.net/manual/en/language.types.string.php | PHP documentation]] were handled correctly.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9182
Summary: Ref T4954. There was a typo in the original implementation. It wasn't noticed until now because the `getVersion` function isn't really used anywhere.
Test Plan: Ran `arc linters` in a repository with `ArcanistJSONLintLinter` configured, noticed that the version number appeared in the output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D9160
Summary: As noticed by @epriestley in D9060, `flake8 --version` can emit a string like "2.0" rather than the expected "2.0.0".
Test Plan: Uninstalled my existing version of `flake8` (v2.1.0) and installed a prior version (`pip install flake8==2.0.0`). Ran `flake8 --version` and inspected the output.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9174
Summary: As identified by @epriestley in D9060, `jshint --version` emits version information on stderr, not stdout.
Test Plan: Ran `jshint --version` locally and verified that the version information is output to stderr.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D9173
Summary: Fixes T5082. We try to access the repository API in some cases when we don't have one.
Test Plan:
- Made revisions with "arc diff --raw-command --create".
- Made this revision, without "--raw-command".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5082
Differential Revision: https://secure.phabricator.com/D9165
Summary:
Add a wrapper around [[http://www.coffeelint.org/ | CoffeeLint]] as an `ArcanistExternalLinter`.
Depends on D9041.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9051
Summary: Fixes T5085. Currently, the `ExecFuture` instances that are used to call an external linter are executed in the current working directory. This means that if a path is specified in the `.arclint` file, relative to the project root directory, that the path will not be properly interpreted by the external linter when `arc lint` is called from a level deeper than the project root.
Test Plan: Ran `arc lint` from a subdirectory of a project and verified that the linter did not throw an exception due to not being able to find the specified configuration file.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5085
Differential Revision: https://secure.phabricator.com/D9159
Summary:
Allow `.jshintrc` and `.jshintignore` paths to be passed to `jshint`.
Ref T2039.
Test Plan: Added the `jshintrc` and `jshintignore` keys to an `.arclint` file that was configured to use `ArcanistJSHintLinter`. Ran `arc lint --trace` and inspected the flags that were passed to `jshint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: john.snow, epriestley, Korvin
Maniphest Tasks: T2039, T5085
Differential Revision: https://secure.phabricator.com/D9112
Summary: Personally, I am a strong fan of this rule. There is currently a similar rule provided by PHP_CodeSniffer.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9117
Summary: Since this method is only used within this class, it makes sense to move it here.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9114
Summary: Fixes T5071. If you run `arc diff --raw` from some arbitrary, non-repository directory we incorrectly try to read information out of the working copy. Even if this information is available, `arc diff --raw` should not assume it's accurate (there's no guarantee the diff comes from the working copy).
Test Plan: Ran `arc diff --raw` in an arbitrary directory.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5071
Differential Revision: https://secure.phabricator.com/D9142
Summary:
Found https://secure.phabricator.com/D4519 but it was horribly out of date, so decided to write a new version.
Kinda clobbered together form that diff and the new version of ArcanistJSHintLinter
Test Plan: Tested on personal projects with js files.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9131
Summary: This fixes an issue with the C# linter where a message could be returned from cslint that wasn't intended for use with parameters. This just ensures there's enough parameters so that it won't crash (and consequently ignore lint messages).
Test Plan: Ran the linter, it didn't crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9116
Summary: Ref T4948. Move the `startDocument` code to a `renderPreamble` function so that, at least theoretically, the renderer can be reused. Otherwise, the only way to reuse the renderer would be to construct a new instance.
Test Plan: Ran `arc lint --output xml` and verified that the output looked reasonable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9108
Summary: Although this provides less context in terms of the error message (for example, `Parameter has invalid type. Expected type 'optional regex|list<regex>', got type 'list<string>'.`), I think that it is the right approach. I think that `PhutilTypeSpec::checkMap` should be improved such that additional context is provided in the exception message.
Test Plan: Ran `arc lint`. Modified `.arclint` to contain an invalid regex and ran `arc lint` again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9089
Summary: The Checkstyle XML output format is not intended to be an interactive workflow.
Test Plan: Introduced linter issues and ran `arc lint`. Verified that no interactive prompt was shown.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9107
Summary: Fixes T4948. Add a lint renderer which supports outputting the lint results in the Checkstyle XML format.
Test Plan: Ran `arc lint --xml` and inspected the output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9083
Summary:
Just some minor cleaning up, including:
- Removing old annotations for Diviner.
- Renaming some `lint-test` files so that the directory name bears a closer resemblance to that of the linter.
- Remove some useless `return` statements.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9096
Summary: Rename this test method for consistency with other linter test classes.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9095
Summary: Arcanist is a dirty rotten liar. I made it less of a dirty, rotten liar.
Test Plan: http://imgur.com/36qXcgI
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9081
Summary: It looks like this command is just hanging if you skim the documentation and miss that you have to echo parameters into it. Print out a hint.
Test Plan: Ran `arc call-conduit` and got a hint.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9050
Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7958
Summary: Ref T2039. The type specs are right in theory but not quite correct in practice, since we pass strings in rather than objects.
Test Plan: While tweaking `phabricator/`, adjusted these to get desirable results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9071
Summary: Ref T2039. It looks like "phutil-library" didn't make it over, I'll add that to the other two (unless I'm wrong and this isn't an oversight?).
Test Plan: Used `arc linters` to read help.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9070
Summary: It seems that there is a lot of overlap between `getConfig` / `setConfig` and `getLinterConfigurationOptions` / `setLinterConfigurationValue` respectively.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9067
Summary: Currently, `PhabricatorLintEngine` configures the `ArcanistPhutilXHPASTLinter` linter. In order to use `.arclint` instead, we need to expose the `ArcanistPhutilXHPASTLinter` configuration to the `.arclint` format.
Test Plan: See D9064.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9066
Summary: Ref T2039. Make the `ArcanistCppcheckLinter` compatible with `.arclint`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9068
Summary:
Ref T2039. I'll update the corresponding documentation.
It feels a little awkward that this is disconnected from `getLinterConfigurationOptions()`, but I dislike returning weird ad-hoc structures more than I dislike having two methods. Most linters don't implement either of these anyway.
Test Plan: Ran `arc linters` and `arc linters --verbose`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9062
Summary: Ref T2039. The `.arclint` file is reasonably complete now and we should start using it if possible, since we are trying to recommend it to others.
Test Plan: `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9057
Summary:
Currently, the Phutil XHPAST Linter and vanilla XHPAST Linter reuse the same parse tree, but do this by having explicit knowledge of one another.
Instead, let them synchronize by writing to a glorified array of globals on the Engine. They no longer require knowledge of one another, so this can work under `.arclint`.
(This could probably be a little cleaner by putting more logic in the shared base class, but Facebook has some kind of goofy subclass of this thing and //this// patch won't disrupt it, while a cleaner one might.)
This should unblock D9057.
Test Plan: Ran unit tests and normal lint, got accurate looking results without duplicate invocations showing up in `--trace`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9059
Summary: The `ArcanistGeneratedLinter` and `ArcanistNoLintLinter` don't actually ever raise any linter messages, so it doesn't make sense to set custom severities for these linters. Instead, don't expose this configuration.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9038
Summary:
Ref T2039. Addresses two issues:
- Issues a warning for use of config which is deprecated by `.arclint`.
- We no longer require an engine to be present for these linters, so `arc linters` doesn't fatal if they aren't configured.
Test Plan:
- Ran `arc linters` in a repo with no JSHint.
- Ran `arc linters` in a repo with junk in .arcconfig and got a warning when it was read. Verified it still took effect.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9058
Summary: Not all linters run on a `command file` fashion. In particular, the maven checkstyle plugin runs like `command --flag=file`.
Test Plan: Run a linter that extends `ArcanistExternalLinter`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9026
Summary: Ref T2039. This isn't exhaustive, but moves things forward by a decent chunk.
Test Plan: Used `arc linters` and read the messages.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9056
Summary: Ref T2039. We're starting to get kind of a lot of linters; provide `arc linters` to help users review and understand them and construct `.arclint` files.
Test Plan: {F152205}
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9041
Summary: Currently, paths to be excluded from linting need to be specified for each linter individually. This is a pain for projects that are using even a moderate number of linters and which have common paths which should be excluded from linting completely.
Test Plan: Unfortunately, it's hard to test this sort of stuff. I cloned the `arclint-examples` repository and tested my changes there,
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9054
Summary: Explicitly specify the types of the function parameters. This change is basically the same as D8388.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9040
Summary: The only real change here is adding a `getLinterConfigurationName` method so that this linter can be used with an `.arclint` file. Everything else is just some minor tidying.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9039
Summary: This isn't really necessary, nor does it bear any significant benefits, however semantically it seems to make sense. From `csslint --help`, the `--quiet` flag causes `csslint` to "Only output when errors are present".
Test Plan:
Tested using `csslint` directly:
```
> csslint test.css
csslint: No errors in /home/joshua/workspace/github.com/facebook/arcanist/foo.css.
> csslint --quiet test.css
```
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9036
Summary: No subclass should need to override these methods. Additionally, none currently do.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9035
