Summary:
Fixes T8674. Currently, `ArcanistSelfMemberReferenceXHPASTLinterRule` warns if a fully-qualified class name is used in place of `self`. This is fine in most cases, but in some specific scenarios fails for PHP 5.3 because `self` (and also `$this`) cannot be used in an anonymous closure (see [[http://php.net/manual/en/functions.anonymous.php | anonymous functions]] and [[https://wiki.php.net/rfc/closures/removal-of-this | removal of `$this` in closures]]).
In order to do this, I also had to modify the manner in which configuration was passed to individual linter rule. Previously, it was only possible or an individual linter rule to be set with a configuration value. Once the linter rule had set this value, there was no means by which to allow this value to be shared amongst linter rules.
Depends on D13819.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8674
Differential Revision: https://secure.phabricator.com/D13820
Summary:
Improve `ArcanistUselessOverridingMethodXHPASTLinterRule` by allowing overriding methods which set default values. For example, the following scenario is perfectly valid:
```lang=php
class SomeClass {
public function __construct($x) {}
}
class SomeOtherClass extends Class {
public function __construct($x = null) {
parent::__construct($x);
}
}
```
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13840
Summary: Ref T8674. Adds to `ArcanistPHPCompatibilityXHPASTLinterRule` such that an error is raised whenever `self` or `$this` is used in an anonymous closure prior to PHP 5.4.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8674
Differential Revision: https://secure.phabricator.com/D13841
Summary: Ref T8742. Anonymous functions should have a space after the `function` keyword. Additionally, ensure that there is a space before and after the `use` token.
Test Plan: Modified existing test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8742
Differential Revision: https://secure.phabricator.com/D13804
Summary: Unary postfix expressions should not have a space before the operator.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13805
Summary: Add a linter rule to check that there is no whitespace surrounding the object operator, `->`.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13798
Summary: Add a linter rule to ensure that there is no space between the operator and the operand in a unary prefix expression.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13797
Summary: Repurpose `ArcanistClosingCallParenthesesXHPASTLinterRule` (and rename it to `ArcanistCallParenthesesXHPASTLinterRule`) to ensure that there is no spacing before opening parenthesis in function calls.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13796
Summary: Ref T7419. This makes it easier to render helpful documentation in the next diff without having to copy/paste things.
Test Plan:
In next diff:
{F688894}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7419
Differential Revision: https://secure.phabricator.com/D13788
Summary: Ref T8921. See similar change in D13695. This expands the scope to also coerce `setChar()`.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T8921
Differential Revision: https://secure.phabricator.com/D13737
Summary:
Fixes T8912. Property `$project_root` was missing in `PytestTestEngine` class, resulting in
broken py.test wrapper. Also renaming the property so the linter is happy.
Test Plan: `arc unit --everything`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: kparal, epriestley, Korvin
Maniphest Tasks: T8912
Differential Revision: https://secure.phabricator.com/D13698
Summary: Fixes T8921. Harbormaster is strict about types it accepts, but `ArcanistLintMessage` is more liberal. Push the strictness barrier down to the linter level, while maintaining reasonable flexibility in the API.
Test Plan: `arc unit --everything`
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8921
Differential Revision: https://secure.phabricator.com/D13695
Summary: "Advice" is not a valid severity for Checkstyle... valid severities are `ignore`, `info`, `warning` and `error`.
Test Plan: Read the [[http://checkstyle.sourceforge.net/property_types.html | documentation]].
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13684
Summary: Improve `ArcanistClosingDeclarationParenthesesXHPASTLinterRule` (and rename it to `ArcanistDeclarationParenthesesXHPASTLinterRule`) to ensure that there is no whitespace before the opening parenthesis of a function/method declaration.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13675
Summary:
This is to fix `arc unit` when running a test file with no test results (e.g. skipped)
```
EXCEPTION: (RuntimeException) Undefined variable: last_test_finished at [<phutil>/src/error/PhutilErrorHandler.php:210]
arcanist(head=master, ref.master=d54cb072facd), deviantart(), phutil(head=master, ref.master=75f675747648)
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<arcanist>/src/unit/parser/ArcanistPhpunitTestResultParser.php:95]
#1 ArcanistPhpunitTestResultParser::parseTestResults(string, string) called at [<deviantart>/unit/DaUnitEngine.php:150]
#2 DaUnitEngine::parseTestResults(string, TempFile, string, string) called at [<deviantart>/unit/DaUnitEngine.php:82]
#3 DaUnitEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:186]
#4 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:382]
```
Test Plan: Create a test file with skipped tests. Run `arc unit`. Make sure the exception is not thrown.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D13640
Summary: Fixes T8805. Explicitly require int/float for duration. Adjust XUnit/Go parsers to provide one.
Test Plan: Ran unit tests.
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: jsotuyod, champo, epriestley
Maniphest Tasks: T8805
Differential Revision: https://secure.phabricator.com/D13637
Summary:
Ref T8332. I find it really odd that I need to run `arc lint --everything --output xml > checkstyle.xml` and feel that it would be much more intuitive to just run `arc lint --everything --output xml` and have the output written to `checkstyle.xml`.
To provide context, we are running `arc lint --everything` in a CI job and parsing the results.
Test Plan: Ran `arc lint --everything --output xml` and saw the output written to file.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8332
Differential Revision: https://secure.phabricator.com/D13570
Summary: I think that this output was used during the early stage of `ArcanistConfigurationDrivenLintEngine`, but I question it's value nowadays. In particular, I find that this output makes the output of `arc lint --trace` significantly less useful.
Test Plan: Ran `./bin/arc lint --trace` and saw useful output.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13593
Summary: This host is no longer in service.
Test Plan: `git grep -i www.phabricator.com`
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13586
Summary: Fixes T8714. When a test engine isn't returning the correct result type, shift suspicion onto it.
Test Plan: Faked error, got exception blaming test engine.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8714
Differential Revision: https://secure.phabricator.com/D13486
Summary:
Ref T8670. Ref T8657.
- When lint only has advice (no warnings/errors), consider it a "passing" build.
- Be a little louder about `sendmessage` calls failing because this stuff is not totally broken and that makes T8670-related things easier to catch/fix.
Test Plan: Created this revision.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8657, T8670
Differential Revision: https://secure.phabricator.com/D13436
Summary: Return `$this` from a bunch of setter methods for consistency. See also D13422.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13423
Summary: For consistency, a single space should separate `catch` and the following parenthetical expression.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13389
Summary:
Ref T8095. Before uploading lint/unit results in the old way, try to ship them to autotargets.
If we can query and upload data to autotargets, do so, and then skip the older style uploads.
Test Plan: Used `arc diff` to ship data up via Harbormaster.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13381
Summary: Add some tests to ensure that `ArcanistXHPASTLinterRule` subclasses are properly implemented. This should catch issues such as two linter rules having the same `ID` value. See D13272 for a similar change.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13274
Summary: All base classes should extend from `Phobject` or some other classes. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13281
Summary: If I understand correctly, all classes should extend from `Phobject`?
Test Plan: This needs some further work
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13275
Summary: I found another issue with T8042... it seems that tests from the root directory (i.e. `src/__tests__` are not being discovered properly). The handling of this case (`$library_path` being the library root directory) can probably be improved, and I am open to suggestions. Depends on D13202.
Test Plan: Added to existing test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13185
Summary: Minor tidying of documentation and adding some groups.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13135
Summary:
Ref T8042. Tests were not being discovered in two different scenarios:
# Files modified at the same level as the library root directory.
# "Normal" directories like `src/unit/engine`.
This regression was caused by D12689.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8042
Differential Revision: https://secure.phabricator.com/D13126
Summary: Fixes T8374.
Test Plan:
```
$ arc diff
You have untracked files in this working copy.
Working copy: /Users/epriestley/dev/core/lib/arcanist/
Untracked changes in working copy:
(To ignore this change, add it to ".git/info/exclude".)
derp
Ignore this untracked file and continue? [y/N] ^C
```
Reviewers: btrahan, joshuaspence, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8374
Differential Revision: https://secure.phabricator.com/D13096
Summary:
The `ArcanistXHPASTLinter` class is becoming quite bloated. This diff separates the class into one-class-per-rule, which makes everything much more modular. One downside to this decoupling is that code reuse between linter rules is much more difficult, although this only affects a very small number of linter rules.
There is still some further work that could be done here, but I defer this until a later diff:
- Rewrite `ArcanistPhutilXHPASTLinter` using `ArcanistXHPASTLinterRule`.
- Change the unit tests so that they are truly only testing a single linter rule.
- Maybe improve the way in which linter configuration options are handled.
- Make it easier to keep track of the linter rule IDs (see T6859).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: johnny-bit, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10541
Summary: Fixes T8344. Prior to D12971, this always returned `array()`. It may now sometimes return `null`. Switch the behavior to be more similar to the old behavior.
Test Plan: Inspection.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8344
Differential Revision: https://secure.phabricator.com/D13061
