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. Provide some examples to help users figure out how to write these things.
Test Plan: Checked them with `jsonlint`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9065
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
Summary: PHPCS does actually support reading data from stdin, so let's make `ArcanistExternalLinter` aware of this.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9034
Summary: Convert the `cpplint.py` wrapper linter to `ArcanistExternalLinter`. This is in preparation for T2039.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9033
Test Plan:
Verified manually. Something like this in .arclint:
"linters" : {
"text" : {
"type" : "text",
"include" : "(\\.(txt|py|html?)$)",
"text.max-line-length": 200
}
changes the line length. Something other than an integer there raises an error.
Reviewers: epriestley, #blessed_reviewers, #arcanist
Reviewed By: epriestley, #blessed_reviewers, #arcanist
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9029
Test Plan: Ran `arc lint` locally. Now it doesn't exit with an error.
Reviewers: #blessed_reviewers, #arcanist, epriestley
Reviewed By: #blessed_reviewers, #arcanist, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9028
Summary: Having a `debug` key in the `.arclint` file format doesn't seem right. Instead, it would be better to just use a `PhutilConsole` and the `writeLog` method so that "debug" messages are output when using `arc --trace`.
Test Plan: Ran `arc lint --trace` in a repository using `.arclint`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9021
Summary: The `lessc` binary is a part of the NPM `less` module.
Test Plan: `npm install less` works whereas `npm install lessc` does not.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9006
Summary: Contine is not a word methinks.
Test Plan: looks better
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8997
Summary: This class provides an adapter for [[https://github.com/less/less.js/ | lessc]].
Test Plan: Wrote and executed unit tests.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8992
Summary:
It seems that in some situations, JSHint does not set the `evidence`
property. In such cases, PHP fails with `Undefined property:
stdClass::$evidence`. It would be safer to access the error object as an
associative array rather than as `stdClass`.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8993
Summary: This linter is a wrapper around [[http://puppet-lint.com/ | puppet-lint[]].
Test Plan: Wrote an executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8990
Summary: These methods are declared `public`, but there are meant to be `protected` (as they are declared in `ArcanistExternalLinter`).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8991
Summary: Add a linter which uses [[http://php.net/simplexml | SimpleXML]] to detect errors and potential problems in XML files.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8989
Summary: Provide bindings for [[https://github.com/zaach/jsonlint | JSONLint]], which is a useful tool for linting and validating JSON. Theoretically, this could be done with pure PHP, however it would not be trivial (`json_decode`, for example, does not provide any context as to JSON validation errors).
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8988
Summary:
This unit test is failing for me. It seems that I have a newer version of `flake8`. We should probably provide support for the most recent versions of any external tools, at least until we can implement version-specific stuff.
I am running `2.1.0 (pep8: 1.5.6, pyflakes: 0.8.1, mccabe: 0.2.1) CPython 2.7.6 on Linux`.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8987
Summary:
Ref T3186. Ref T2039.
- Convert JSHint to modern format; improve granularity of errors.
- Convert PyFlakes to modern format;
- Remove ApacheLicenseLinter and LicenseLinter (these have been deprecated for a very long time).
This is somewhat disruptive and will break some users by no longer respecting various path/config options. I'll sequence documentation and deprecation warnings in front of these.
Test Plan: Ran unit tests.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, joshuaspence, aran
Maniphest Tasks: T3186, T2039
Differential Revision: https://secure.phabricator.com/D6810
Summary:
This method will, theoretically, allow `arc lint` to be configured to require some minimum version of an external linter (although this would probably require significantly more work).
Additionally, the existence of this method simplifies the `getCacheVersion` function which, previously, was implemented by the external linters individually. Instead, a general approach to determining the version for cacheing purposes can be used.
Fixes T4954.
Test Plan: I'm not sure how to test this.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D8971
Summary: Modernize `ArcanistJSHintLinter` by extending from `ArcanistExternalLinter` instead of `ArcanistLinter`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8965
Summary:
Fixes T4938. Now that we have customs statuses, this workflow needs to know about 'em.
One fun caveat is that Conduit isn't up and running to generate the help method, so I had to add a parameter to have the statuses list out. This could maybe be omitted since entering erroneous status options does indeed tell you the correct options.
Test Plan:
`arc close --list-status` -- got a list of statuses
`arc close T1` -- closed T1
`arc close T1` -- error T1 closed already
`arc close T1 -s foobar` -- T1 status set to foobar
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1, T4938
Differential Revision: https://secure.phabricator.com/D8938
Summary: Like git, it is also valid to have empty net change under hg. Should not throw if that is the case.
Test Plan: Tried on a hg repo.
Reviewers: lifeihuang, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8888
See: <https://github.com/facebook/arcanist/pull/162>
It looks like the word `argument` appears many more times than `augment`, so I'm assuming `agument` is most likely to be a typo of `argument`.
Reviewed by: epriestley
Summary: Personally, I prefer to specify command lines flags as an array rather than a string.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8387
Summary:
Fixes T4809. When landing a revision, check for a (non-manual) buildable of the current diff. If we find one, check its status:
- If it passed, print out a message to inform the user that we checked.
- If it failed or is still building, print out details about the issue and require a confirmation to continue.
- Just ignore other cases.
Test Plan:
- Ran `arc land` on a revision with no buildable, a passing buildable, a failed buildable, and a building buildable for the current diff.
- Got sensible output / prompts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8801
Summary: Fixes T4605. Smart waits (see D8782) are presumably good on the balance, but may cause some delays when changes are made to rarely updated repositories. To help mitigate this, have `arc land` hint that a repository has changed.
Test Plan: Will run `arc land`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8783
Summary: make it a real member variable. See rARC77a9c1814063 and D8753#33914.
Test Plan: sending up this diff!
Reviewers: shadowhand, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8761
Summary:
Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now.
I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be.
Also a modest amount of bonus pht.
Test Plan: this very diff. i'm going to arc land it laters too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4163
Differential Revision: https://secure.phabricator.com/D8753
Summary: Fixes T4291. Also pht user-facing strings.
Test Plan:
made a branch `foo` off master. made a commit and a diff in `foo`. switched backed to master and cowboy committed some thing. went back to branch`foo`, did an arc land, and saw the error message. went back to master, did a git resert --hard HEAD^1, went back to branch `foo`, and then successfully arc landed.
also ran arc help land and things looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4291
Differential Revision: https://secure.phabricator.com/D8738
Summary:
I changed the rendering of the bar color for the `priority` column when running `arc tasks` to match the `priorityColor` property.
If the `priorityColor` property is one of the basic colors already supported by ansi, then the bar is set to that color, otherwise it is set to white.
This will allow the user to customize maniphest priorities and then set their own colors and have those colors display correctly when running `arc tasks`
Fixed some linting errors
Test Plan:
Run `arc tasks` and ensure that:
* the priority column is displayed with a colored bar
* the priority column bar is the correct color (or white if it is an unsupported color)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8735
Summary: I changed the way `arc tasks` was checking to see if a maniphest task was closed from looking for a non-empty status to looking for the isClosed property submitted in D8731.
Test Plan: Run `arc tasks` and check that the tasks show open/closed correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8732
Summary:
We recently moved our HEAD and it caused some issues on `arc patch` with git-svn repos. The base revision is incorrect and patch will fail. Add the check in such case to make it work.
The check was there before but removed in change b202158. The reason wasn't mentioned there though.
Test Plan: Tried it on svn.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8717
Summary: Ref T4670.
Test Plan: arc patch D8685; arc land --hold; verified i got a nice message asking me to be sure i wanted to land epriestley's code
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4670
Differential Revision: https://secure.phabricator.com/D8709
