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: Apply the `ArcanistChmodLinter` from D9187.
Test Plan: Removed all other linters from the `.arclint` file and executed `arc lint --everything`. Confirmed that there were no issues raised.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9190
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: We have old/inconsistent code for reading these. Instead, read from sources in a modern way.
Test Plan:
- Removed "default" and "phabricator.uri" from user/local/project config.
- Set "default" in "/etc/arcconfig".
- Ran `arc tasks` and got a "connect to Phabricator" message.
Reviewers: btrahan, davedash
Reviewed By: davedash
Subscribers: davedash, epriestley
Differential Revision: https://secure.phabricator.com/D9105
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
