Summary:
- Move name helper functions to ArcanistXHPASTLintNamingHook to make it easier
to write custom linters.
- Add test coverage for name functions.
- Add 'variable' and 'global' naming convention tests.
- Expand test cases.
- Improve lint message error when an unexpected message is raised during a
test.
- Remove a defunct XHP lint message.
Test Plan:
- Ran unit tests.
- Ran "arc lint --lintall" on arcanist/.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: johnduhart, aran, epriestley, arudolph
Differential Revision: https://secure.phabricator.com/D1506
Summary:
This is a fancy version of "land.sh" that uses "git merge --squash" and
"arc which" to cover more cases.
Test Plan:
Ran "arc land" against various repository states (no such branch, not
accepted, valid, etc). Things seemed OK. There are basically an infinite number
of states here so it's hard to test exhaustively.
Reviewers: cpiro, btrahan, jungejason, davidreuss
Reviewed By: davidreuss
CC: zeeg, aran, epriestley, davidreuss
Maniphest Tasks: T787, T723
Differential Revision: https://secure.phabricator.com/D1488
ArcanistRepositoryAPI->loadWorkingCopyDifferentialRevisions()
Summary:
- See T787.
- @cpiro has an immediate use case for this, which is ##arc amend --revision
`arc which --id` --show master## for "git merge --autosquash" or similar.
- For T614, we need this to choose "--create" vs "--update".
- Other workflows should also use this to improve how often we automatically
get things right, particularly in Mercurial and SVN.
Test Plan:
Ran "arc which" in SVN, Git and HG working copies with various flags;
results seemed reasonable.
Reviewers: btrahan, cpiro, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T787
Differential Revision: https://secure.phabricator.com/D1478
Summary:
Julien built a really cool static analysis database of our codebase. One
capability is that it can suggest typehints that are not in the code. The
analysis to do this is very expensive, so it can't reasonably be run locally.
But it can remain indexed on a server.
The idea here is to provide a familiar interface to it through arc lint, via a
generic Conduit service call.
In our lint engine, this will probably be gated on --advice for performance.
This will introduce a slight awkwardness in that running with --advice can add
new non-advice lint if the server chooses, but this isn't likely to cause a
practical problem.
Test Plan:
Construct a fake Conduit lint endpoint, attach this linter to it, and see bogus
lint
appear with --advice.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1462
Summary: D1439 added a new class which had the wrong name in the map from D1439.
Whoopsiepoos!
Test Plan: unit tests now pass
Reviewers: epriestley, zeeg
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1441
Summary:
This adds a new lint engine, ComprehensiveLintEngine, which
includes sane defaults for some generic languages.
Ideally this would include *all* available language linters,
but that can be enhanced at a later point. Right now it's mostly
the base linter with additional JavaScript and Python linters.
Test Plan:
Adjust the lint_engine to be "ComprehensiveLinterEngine". You'll
also need jshint, pyflakes, and pep8 to all be available on PATH.
Run arc lint against files which contain .php, .py, and .js.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1439
Summary:
Inspired by http://news.ycombinator.com/item?id=3464671 and a lot of
diffs I've seen @ FB, I've added a spell checking linter. To reduce
false positives, it's only a blacklist. Still, it catches a large
number of 'issues'.
Test Plan:
Unit tests. Ran on FB's codebase. No false positives
noticed but a lot of cases caught.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, jack
Differential Revision: https://secure.phabricator.com/D1409
This patch adds ArcanistJSHintLinter class and three new .arcconfig options:
* lint.jshint.prefix - directory where JSHint binary resides
* lint.jshint.bin - JSHint binary name (if different from 'jshint')
* lint.jshint.config - a JSON file with JSHint project-wide options
By default, this linter assumes that JSHint is installed on user's system
as an NPM package.
Test Plan:
(1)
Run `npm install jshint -g` to install JSHint and add
ArcanistJSHintLinter to your Lint Engine (I didn't see PEP8 or
PyFlakes in the PhutilLintEngine so I decided to not to put
JSHint in there). After that you can do `arc lint my.js`.
(2)
Create a new file, config.json, and add `{ "white": true }` in it.
Add `"lint.jshint.config": "/path/to/config.json"` to your .arcconfig and
run `arc lint my.js`. After that, unless your name is Douglas Crockford,
you'll see tons of JSHint warnings about minor PEP8-like things (spacing, etc.)
revision is the correct base revision relative to the patch.
Summary: What the title says. If not correct, warn the user. This check
honors the --force flag to skip all these checks. This change also includes
moving some Differential constants into Arc so they can be used for both
projects. There is a corresponding phabricator diff (incoming) to address this
part of the change.
Test Plan:
For a project with actual diffs, a git repository tracked by phabricator, *AND*
development in master branch only, do some...
- git reset --hard HEAD^1
- arc patch DX, where X is what got us to HEAD in the first place
- verify successful patch
...then...
- git reset --hard HEAD^^
- arc patch DX, where X is what got us to HEAD in the first place
- verify warning
- verify Y versus N continues versus stops appropriately
Note if development were done outside the master branch this warning message
will fire early / often as git commit hashes are based on the commit *and* the
rest of the source code the commit is made against. This is (unfortunately) the
"typical" case so this warning is pretty active at the moment. T201 will
eventually land and when parsing a given commit update the corresponding diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1328
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Test Plan:
Added a test event listener, added an event dispatch in diff workflow, run the
workflow, the listener was called.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 1102
Summary:
We used square distance that optimized for the wrong thing. Making the same
markers spread out instead of being together
Also added a very little cost for switching type. That will make diff types
stick together a bit more
Task ID: #623
Blame Rev:
Test Plan:
Ran my new unit test. And tested a few diff in phabricator
Revert Plan:
Tags:
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1112
Summary: 'patch' chokes on hunks with too much trailing stuff. 'git apply'
chokes on overlapping hunks. Make them both happy. Write some test cases so I
stop breaking this stuff.
Test Plan:
- Applied a previously-failing patch via SVN.
- Applied a previously-failing-then-succeeding patch via Git.
- Ran unit tests.
Reviewers: jungejason, btrahan, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1099
Summary: Move code to actually parse "hg" output into a separate class with some
tests, so I can reuse it in the import scripts. We should probably do this for
Git/SVN at some point, too.
Test Plan: Ran unit tests, used this class in Phabricator importers, grepped for
calls to removed private methods.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 942
Summary:
This should support conservative rewrite policies in git fairly well, under an
assumed workflow of:
- Develop in local branches, never rewrite history.
- Commit with "-m" or by typing a brief, non-template commit message
describing the checkpoint.
- Provide rich information in the web console (reviewers, etc.)
- Finalize with "git checkout master && arc merge branch && git push" or some
flavor thereof.
This supports Mercurial somewhat. The major problem is that "hg merge" fails if
the local is a fastforward of the remote, at which point there's nowhere we can
throw the commit message. Oh well. Just push it and we'll do our best to link
them up based on local commit info.
I am increasingly forming an opinion that Mercurial is "saftey-scissors git".
But also maybe I have no clue what I'm doing. I just don't understand why anyone
would think it's a good idea to have a trunk consisting of ~50% known-broken
revisions, random checkpoint parts, whitespace changes, typo fixes, etc. If you
use git with branching you can avoid this by making a trunk out of merges or
with rebase/amend, but there seems to be no way to have "one commit = one idea"
in any real sense in Mercurial.
Test Plan: Execute "arc merge" in git and mercurial.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 860
Summary:
See T326. Allow lint rules to be selectively overridden, e.g. for Conduit
methods.
Since FB has a long history of suggesting crazy patches for this stuff I think
we're safer just adding a hook class than trying to do some kind of regexp
magic.
Test Plan: Wrote a hook for Phabricator and linted some Conduit files without
issues. Ran unit tests.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 874
Summary:
There's a lot of ground left to cover but this makes "arc diff" work (on one
trivial diff) in my sandbox, at least, and supports parsing of Mercurial native
diffs (which are unified + a custom header). Piles of missing features, still.
Some of this is blocked by me not understanding the mercurial model well yet.
This is also a really good opportunity for cleanup (especially, reducing the
level of "instanceof" in the diff workflow), I'll try to do a bunch of that in
followup diffs.
Test Plan: Ran "arc diff" in a mercurial repository, got a diff out of it.
Reviewed By: aran
Reviewers: Makinde, jungejason, tuomaspelkonen, aran, codeblock
CC: aran, epriestley, codeblock, fratrik
Differential Revision: 792
Summary: Read and write in the same workflow! Dogs and cats living together!
Test Plan: - Performed a bunch of paste reads and writes and they looked ok?
Reviewed By: aran
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 748
Summary: Mechanisms for interacting with Files via Arcanist.
Test Plan:
- Ran 'arc upload x', 'arc upload x y z'
- Ran 'arc download' with --as and --show.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 742
Summary:
Document the relationship between lint engines and linters. Provide an example
linter. Improve the documentation of PyLintLinter, which has a bunch of
configuration stuff which you had to dig into the code to get.
Test Plan:
Ran "arc lint --engine ExampleLintEngine --lintall derp.py" on a file with a
Python syntax error in it. Read documentation.
Reviewed By: j3kuntz
Reviewers: j3kuntz, andrewjcg
CC: aran, j3kuntz
Differential Revision: 557
Summary:
Appending differential status, sorting, filtering and coloring git
branches.
I think it turned out rather nicely. On my repository with 70 branches
it takes 1.6s, not terrible, though 1.2s is in the conduit call - seems
like there is potential for optimization.
I didn't end up changing 'arc list', as their semmantics are slightly
different, but I'm open to ideas of consolidating them
Test Plan:
- Tested on both facebook www and arcanist repositories.
- Validated that view-all flag works
- Validated that the ordering is correct
- Validated that the statuses match the differential status.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 497
Summary:
Provide an "install-certificate" workflow to simplify ~/.arcrc edits. See also
D460.
Test Plan:
Installed certificates via "arc install-certificate".
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 465
Summary:
Provides a lint class as a wrapper around the external project PyLint.
This exposes some arc config variables to control the behavior:
lint.pylint.prefix - non-standard installation location of pylint
lint.pylint.logilab_astng.prefix - non-standard installation location
of logilab-astng, a dependency of pylint
lint.pylint.logilab_common.prefix - non-standard installation location
of logilab-common, a dependency of pylint
lint.pylint.codes.{error,warning,advice} - regexes matching against
PyLint message codes which should trigger arc errors/warnings/advice
lint.pylint.options - options to pass PyLint
Test Plan:
used to lint python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 343
Summary:
Provide a simple linter wrapper around pyflakes. This relies on finding
pyflakes via:
- lint.pyflakes.path - arcconfig setting of absolute path to pyflakes
- lint.pyflakes.prefix - arcconfig setting of the prefix that pyflakes
was installed under
- users path
Test Plan:
linted python code with PyFlakes warnings
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, andrewjcg, jungejason
Differential Revision: 310
Summary:
The story for creating and maintaining libphutil libraries and modules
is pretty terrible right now: you need to know a bunch of secret scripts and
dark magic. Provide 'arc liberate' which endeavors to always do the right thing
and put a library in the correct state.
Test Plan:
Ran liberate on libphutil, arcanist, phabricator; created new
libphutil libraries, added classes to them, liberated everything, introduced
errors etc and liberated that stuff, nothing was obviously broken in a terrible
way..?
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 269
Summary:
Provide a formal mechanism for making conduit calls from other scripts.
Test Plan:
Called 'conduit.ping'.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 239
Summary: The multi-line comment regexp was potentially too greedy. See
"greedy.lint-test".
- Made it less greedy.
- Added test coverage.
- Fixed an issue with the Apache license getting applied with too much
whitespace against C files.
Test Plan: Ran unit tests.
Reviewers: aran
CC:
Differential Revision: 36
Summary:
Adds data-driven shell completion help to arcanist.
Test Plan:
ran various commands in git and svn working copies,
output seemed reasonable
Differential Revision: 201754
Reviewed By: adonohue
Reviewers: mroch, adonohue
Commenters: crackerjack
CC: epriestley, adonohue, achao
Revert Plan:
OK
