Summary:
Separate XHPAST linter rules in isolation from other rules and formalize the concept of a "linter rule". As the number of XHPAST linter rules grows (we currently have around 80), it becomes increasingly difficult to manage all of the test cases because `ArcanistXHPASTLinterTestCase` currently tests the entire linter (i.e. //all// linter rules) rather than testing individual rules in isolation. See D13534 for a situation in which this is painful. This is particularly bad for third party development because unit tests could break at any time depending on upstream changes.
Basically, in order to facilitate the unit testing of XHPAST linter rules in isolation, I have made the following changes:
# Added a `setRules()` method to `ArcanistXHPASTLinter`. Currently, `ArcanistXHPASTLinter` loads all `ArcanistXHPASTLinterRule` subclasses unconditionally. The `setRules()` method provides a way to override the configured linter rules.
# Formalize the concept of a "linter rule". The `ArcanistXHPASTLinterRule` class was introduced in D10541. I feel that the modularization of `ArcanistXHPASTLinter` has made the linter much more maintainable and easily testable and I intend to extend this concept to other linters, such as `ArcanistTextLinter`.
Test Plan: Ran unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14010
Summary: I've been thinking about this for a while... why not just fold `ArcanistPhutilXHPASTLinter` into `ArcanistXHPASTLinter`?
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13867
Summary: Ref T8742. As mentioned in D13512. This still needs some work, but looks roughly how I expect it to. Mainly, I want to move the standards stuff to the linter itself rather than the linter rule. I wanted to push this out for some initial feedback though.
Test Plan: This still needs work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8742
Differential Revision: https://secure.phabricator.com/D13942
Summary:
Ref T9661. I need to reuse this to fix the complex workflow described in T9661 where we need to follow multiple paths to the upstream and cascade updates across them.
Pull the logic into a separate class to make this easier and less copy/pastey.
This shouldn't change any behavior.
Test Plan: Ran `arc land --preview` from detached head, remote-tracking branch, non-tracking branch, local-tracking branch. Selection of target/remote seemed correct in all cases.
Reviewers: chad
Reviewed By: chad
Subscribers: edibiase
Maniphest Tasks: T9661
Differential Revision: https://secure.phabricator.com/D14360
Summary:
Ref T3855. Fixes T9537. Fixes T8620. Fixes T4333.
This declares bankruptcy and replaces the entire `arc land` workflow under Git. These are the notable changes:
- (T3855) You can now land from a branch to itself.
- (T3855) We now try to restore the original state very aggressively after any failure, instead of dumping you into the middle of a mess.
- (T9537) You can now land without a local branch.
- ([not actually] T9543) We'll now ignore the local branch if it just happens to be named the same thing as the remote branch but doesn't actually track it.
- (T8620) You can now land from a detached HEAD.
- (T4333) We now preserve the author and author date of whatever you land.
This may need some followup work. In particular:
- The signal handler (that tries to put you in a better place if you ^C in the middle of things) causes ^C to work awkwardly in prompts. This might not be worth it.
- Errors/instructions on push/merge issues might need work.
- I dropped support for `--delete-remote` and `--update-with-blah-blah` because I think these flags aren't worth their complexity.
- I've simplified the update/merge algorithm a bit. It may need some complexity added back in.
- I probably missed a few things because this covers like 200 unique, creative workflows.
- Users might need more guidance on the workflows that drop them in the middle of nowhere if they manage to reach them more often than I think.
Test Plan:
- Used `arc land` to land like at least 15,000 different kinds of changes.
- Landed normally.
- Landed from a branch onto itself.
- Landed from a detached head.
- Landed nothing.
- Landed with no local branch.
- Landed onto made-up branches.
- Landed with bad targets.
- ^C'd things in the middle.
Reviewers: chad
Reviewed By: chad
Subscribers: tycho.tatitscheff
Maniphest Tasks: T3855, T4333, T8620, T9537, T9543
Differential Revision: https://secure.phabricator.com/D14356
Summary: Adds two different linter rules (one general purpose and another PHP specific) to prevent empty files from being added to a repository. For some unknown reason, people seem to like to do this.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: avivey, cburroughs, Korvin
Differential Revision: https://secure.phabricator.com/D13881
Summary: `<?php\n\n...` is much easier to read than `<?php\n...`. Depends on D13889 and D13890.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13534
Summary: Adds a basic linter for ensuring that `composer.lock` files are up-to-date.
Test Plan: We have been using this in a private project for around a month.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: johnny-bit, Korvin
Differential Revision: https://secure.phabricator.com/D13883
Summary: Ref T7409. Global variables are gross and should be avoided like the plague.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D13882
Summary: Add a linter rule to prevent trailing commas in a list assignment. `list($x, $y,)` is equivalent to `list($x, $y)`, but the latter is cleaner and should be preferred.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13870
Summary:
Ref T8096. Each test reports coverage information, which we sometimes merge into a combined coverage report.
Usually, each test will report results for every line in the file, so if the file is 30 lines long, coverage is usually 30 characters long.
However, for whatever reason, tests might report results for only the first part of the file. This is allowed and we handle it properly.
Right now, if one test reports 10 lines of results and another reports 30 lines of results, we only use the first 10 lines of results. Instead, extend the merged coverage to include the extra 20 lines of results.
(This is an uncommon case which I only hit because I was manually banging on my keyboard to generate test data, but there's no reason not to handle it better.)
Test Plan: Used web UI, added + executed unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13854
Summary: Add a linter rule to ensure that array elements occupy a single line each.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13842
Summary: Ref T5568. As discussed in IRC. This is very rough and not widely useable, but represents a solid first step.
Test Plan: Ran `arc unit` with a bunch of flags.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rfreebern, aripringle, jaydiablo, BYK, tycho.tatitscheff, epriestley, Korvin
Maniphest Tasks: T5568
Differential Revision: https://secure.phabricator.com/D13579
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: 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: 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:
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:
Ref T8259. Currently, `arc upload` uses new logic but `arc diff` uses older logic internally. This prevents `arc diff` from uploading files larger than 4MB to newer servers.
Split the upload logic apart so the two upload pathways can share it. Callers now build a list of FileDataRefs and hand them to an Uploader for upload.
Test Plan:
Ran `arc upload` on:
- One file.
- Several files.
- Small files.
- Big files.
- Directories.
- Unreadable files.
- Files full of random data.
- The same file over and over again.
- The same big file over and over again.
- Artificially broke `file.allocate` and redid some of the simple cases (large/chunked aren't expected to work in this case).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8259
Differential Revision: https://secure.phabricator.com/D13016
Summary: Ref T7977. The `ArcanistTestCase` class is pointless and can be replaced by `ArcanistPhutilTestCase`. Furthermore, it sorta makes sense to just rename `ArcanistPhutilTestCase` to `PhutilTestCase`. Depends on D12664 and D12666.
Test Plan: `arc unit`
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12665
Summary: This linter is a relic from Facebook days. As Harbormaster becomes more useful (T1049) this linter should become obsolete. Instead of modernising this linter to be compatible with modern infrastructure, it is easier to just let it die.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10533
Summary: Ref T5655. After D9983, `ArcanistBaseWorkflow` and `ArcanistBaseUnitTestEngine` are deprecated.
Test Plan: Wait a sufficient amount of time before landing this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D10004
Summary:
Ref T2039. Convert the `ArcanistPylintLinter` to an `ArcanistExternalLinter` and make it compatible with `.arclint`. In doing so, it was necessary to drop support of older versions of `pylint` by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an `ArcanistMissingLinterException` is thrown.
One caveat here is that support for `lint.pylint.codes.(error|warning|advice)` is dropped. Any installs that are relying on this configuration will need to migrate to using the `.arclint` file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.
Test Plan: Wrote and executed unit tests with `arc unit`. I ran the unit tests on all `pylint` releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9109
Summary: Remove the `ArcanistUncommittedChangesException` class which is unused after D11843.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12515
Summary: Ref T5655. This class was deprecated in D11673.
Test Plan: Wait a few months.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11740
Test Plan:
Add the following JSON to your `.arclint`:
```lang=json
"rubocop": {
"type": "rubocop",
"include": "(\\.rb$)"
}
```
Then, add a ruby file with errors, for instance:
```lang=ruby
def hello()
puts 'world'
end
```
Run `arc lint`. It should come up with something along the line of: "Omit the parentheses in defs when the method doesn't accept any arguments."
Reviewers: joshuaspence, remon, #blessed_reviewers, epriestley
Reviewed By: joshuaspence, #blessed_reviewers, epriestley
Subscribers: reu, calfzhou, jjooss, cburroughs, chad, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10738
Summary:
Ref T7152. Ref T1139.
- Tweak API.
- Move translations out of __init__ file.
Test Plan:
- Ran `arc`.
- Added a goofy translation and made sure it was working.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11746
Summary: Ref T5655.
Test Plan: Ran `arc lint` with `lint.engine` set to `ComprehensiveLintEngine` and saw a deprecation notice.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11673
Summary: I don't think that this provides too much value. I think that we should rework this to be inferred from the `.arcconfig` file perhaps?
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11662
Summary: With a special guest appearance from `ArcanistGeneratedLinter`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11345
Summary: The `src/lint/linter` directory is a bit cluttered at the moment.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11309
Summary: I feel that we are abusing `ArcanistUsageException`. Throw a more tailed exception instead. Depends on D11197.
Test Plan: `arc lint`, I guess.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: avivey, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11205
Summary: Ref T5655. `PHPUnitTestEngineTestCase` is the test case for `PhpunitTestEngine`.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D11203
Summary: Creates a new base class for unit testing `ArcanistExternalLinter` subclasses. Specifically, add a test case for verifying that we are correctly parsing the output of `$external_linter --version`.
Test Plan: `arc unit`
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11197
