Summary: I don't feel that this linter rule belongs in the `ArcanistTextLinter`. In fact, this linter rule is quite similar to the rules provided by `ArcanistGeneratedLinter` and `ArcanistNoLintLinter` and these classes could possibly be consolidated. I have moved this linter rule to a standalone `ArcanistCommitLinter` class (which could possibly do additional lints in the future).
Test Plan: Moved existing test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10473
Summary: Self-explanatory. This is implied anyway as we iterate over it with a `foreach` loop.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11174
Summary: This doesn't make too much sense anymore as all modern linters are configured via the `.arclint` file rather than `.arcconfig`.
Test Plan: `grep`ped to make sure this option isn't used.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11173
Summary: Fix a variable name to be consistent with naming conventions.
Test Plan: `arc lint`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11140
Summary: Currently we output "Waiting for JSON parameters on stdin...", even if `stdin` is piped (for example, from `echo`).
Test Plan:
```lang=bash
> echo '{}' | arc call-conduit conduit.ping
{"error":null,"errorMessage":null,"response":"ip-10-161-81-110"}
> arc call-conduit conduit.ping
Waiting for JSON parameters on stdin...
{}
^D
{"error":null,"errorMessage":null,"response":"ip-10-161-81-110"}
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11122
Summary: The `file.uploadhash` method was added a long time ago (in D4899). It should be safe to assume that this method exists on most installs.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11118
Summary: Ref T5112. The `arc inlines` workflow no longer works because the `differential.getrevisioncomments` API method has been deprecated (see T2222). The command is basically useless right now.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5112
Differential Revision: https://secure.phabricator.com/D9464
Summary:
- The "exclude static variable access" branch in XHPAST incorrectly excluded all variables in a function or method which used any static access (we were selecting the wrong root node for exclusion).
- The "PHPLinter" regexp did not work with 5.4.30. Make it more permissive in what it parses.
Test Plan:
- Added a failing unit test for the variable case and made it pass.
- PHPLinter tests now pass under 5.4.30
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11087
Summary: This unit test is failing locally for me, using v1.1.0 of `puppet-lint`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11057
Summary: Fixes T6773. See also the shameful `PhabricatorDestructableInterface` debacle.
Test Plan: go go dictionary power
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6773
Differential Revision: https://secure.phabricator.com/D11033
Summary:
Ref T5955. If the server supports token-based authentication, prefer it over certificate-based authentication.
Also fixes T3117.
Test Plan:
- Used `arc install-certificate` to install credentials from both token-based and certificate-based hosts.
- Used `arc list` with a token-based host.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3117, T2878, T5955
Differential Revision: https://secure.phabricator.com/D10988
Summary: Extend the `ArcanistXHPASTLinter::LINT_BRACE_FORMATTING` rule to support a bunch of extra cases.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10572
Summary: Extend the `ArcanistXHPASTLinter::LINT_BRACE_FORMATTING` rule to raise a warning when `n_STATEMENT` is used without having `n_STATEMENT_LIST` as a parent. Essentially, this means that `if ($x) { do_y(); }` is preferred over `if ($x) do_y();`.
Test Plan: Added some test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10571
Summary: Fixes T6627. This class documentation is out of date and somewhat misleading in the modern environment.
Test Plan: Read it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6627
Differential Revision: https://secure.phabricator.com/D10899
Summary: This adjusts the behavior of PytestTestEngine so that it correctly reports test failures natively, rather than throwing a command error.
Test Plan: Yeah right
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10866
Summary: Extend `ArcanistXHPASTLinter::LINT_KEYWORD_CASING` to include the `class` keyword.
Test Plan: I can't really added a test case for this without getting "XHP19 Class-Filename Mismatch".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10653
Summary: Minor change, self-explanatory... this seems to make sense.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10476
Summary: Adds an `ArcanistXHPASTLinter::LINT_CONSTRUCTOR_PARENTHESES` rule which ensures that (possibly empty) parentheses are used when calling a constructor.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10597
Summary: In PHP 5.4, the `break` and `continue` statements no longer accept variable arguments (e.g., `break 1 + foo() * $bar;`). Static arguments still work, such as break 2;. As a side effect of this change `break 0;` and `continue 0;` are no longer allowed.
Test Plan: Added some test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10567
Summary: It is not correct to add a trailing comma if the `n_ARRAY_VALUE` node is a heredoc string.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10569
Summary: The `LINT_ARRAY_SEPARATOR` rule was added to `ArcanistXHPASTLinter` in D10535. Unfortunately, it does not correctly handle nested arrays correctly. This patch fixes this. Depends on D10554.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10558
Summary:
Adds a rule to `ArcanistXHPASTLinter` which ensures that:
# Single-lined arrays //do not// contain an unnecessary trailing comma separator after the final array value.
# Multi-lined arrays //do// contain a trailing comma seperator after the final value value.
Depends on D10534.
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/D10535
Summary: I had assumed that we would always be interested in `n_STATEMENT_LIST`, which is fine for `if (...) { some_statements(); }` but not for `if (...) one_statement();`.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10551
Summary: Improve the behavior of `ArcanistTextLinter` in dealing with `LINT_DOS_NEWLINE`. In particular, provide replacement text so that these linter issues can be autofixed.
Test Plan: Added a unit test.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10472
Summary: Explicitly list PHP magic methods (methods beginning with `__`) instead of assuming that //all// methods beginning with `__` are okay in terms of naming conventions. These magic methods were obtained from http://us1.php.net/manual/en/language.oop5.magic.php.
Test Plan:
```
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
1 <?php
2
3 class Foo {
>>> 4 function __foo() {}
5
6 function _bar() {}
7 }
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
3 class Foo {
4 function __foo() {}
5
>>> 6 function _bar() {}
7 }
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10539
Summary: Self-explanatory, improve test coverage.
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/D10474
Summary: It seems that `severity.rules` does not override the default linter severities set by the `ArcanistLinter` itself. This seems incorrect and counter-intuitive.
Test Plan: Created a `"severity.rules": { "(.*)": "disabled" }"` configuration and verified that it was being applied.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10475
Summary: D10132 improved this check by eliminating some false postives, but assumes an `n_FUNCTION_CALL` always starts with `n_SYMBOL_NAME`. This is true for `f()`, but not true for `C::m()` or `$v()`.
Test Plan: Added and ran unit tests.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10467
Summary:
Fixes T5299. Currently, linting `src/future/http/HTTPSFuture.php` from rPHU will raise the following error:
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `curlfile` was not introduced until
PHP 5.5.0.
532 // use this "@" stuff.
533
534 if (class_exists('CURLFile')) {
>>> 535 $file_value = new CURLFile((string)$tmp, $info['mime'], $info['name']);
536 } else {
537 $file_value = '@'.(string)$tmp;
538 }
```
However, since this class is being used conditionally, it //should// be fine and no linter errors should be being raised.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5299
Differential Revision: https://secure.phabricator.com/D10132
Summary: Fix linting for spacing around default parameter assignment in function/method declaration so that `function foo($x = null) {}` is preferred in favor of `function foo($x=null) {}`.
Test Plan: Modified existing unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10429
Summary: Adds an XHPAST linter rule for empty block statements. Basically, if a block statement is empty then it is much neater if the opening (`{`) and closing (`}`) braces are adjacent. Maybe this is just my own personal preference, in which case we could reduce the default severity to `ArcanistLintSeverity::SEVERITY_DISABLED`.
Test Plan: Wrote unit tests. I had to modify a bunch of existing unit tests accordingly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10434
Summary: Add some missing function parameter typehints to the `ArcanistPhutilXHPASTLinter` class.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10430
Summary: `pht`ize some string in `ArcanistPhutilXHPASTLinter` so that they are translatable.
Test Plan: Not 100% sure that I am doing it right...
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10431
Summary: the call to setHeadCommit() was accidentally placed in a `if ($background) {}` block, which was removed in 54bea94. This adds the removed call.
Test Plan: used --head, worked as expected
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10447
Summary:
Calling `arc patch` on a diff that's dependent on a different diff tries to patch the parent first.
To patch the parent a child workflow is created, and a conduit is passed down, but the credentials are not and it is not marked as authenticated.
Then when the child tries to get the commit message for the dependency, it checks isConduitAuthenticated() https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/ArcanistPatchWorkflow.php;2c3268f03ed70d3221eb1642bIc99ebb39b12902e$800 and on failure pops up an interactive editor for the commit message.
Instead we just pass down the credentials to the childred and mark them as authenticated, so this is not a problem.
Test Plan: With two diffs where DA2 depends on DA1, run `arc patch --force --nobranch DA2` ... this no longer pops an interactive editor for the commit message for the dependency.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, seshness, epriestley, Korvin
Maniphest Tasks: T5986
Differential Revision: https://secure.phabricator.com/D10381
