diff --git a/.arclint b/.arclint index c7a8fc950..cc4f7b11e 100644 --- a/.arclint +++ b/.arclint @@ -1,89 +1,90 @@ { "exclude": [ "(^externals/)", "(^webroot/rsrc/externals/)", "(/__tests__/data/)" ], "linters": { "chmod": { "type": "chmod" }, "filename": { "type": "filename" }, "javelin": { "type": "javelin", "include": "(\\.js$)", "exclude": [ "(^externals/JsShrink/)", "(^support/aphlict/)", "(^webroot/rsrc/externals/raphael/)" ] }, "jshint": { "type": "jshint", "include": "(\\.js$)", "exclude": [ "(^externals/JsShrink/)", "(^webroot/rsrc/externals/raphael/)" ], "jshint.jshintrc": "support/jshint/jshintconfig" }, "json": { "type": "json", "include": [ + "(^src/docs/book/.*\\.book$)", "(^support/jshint/jshintconfig$)", "(^\\.arcconfig$)", "(^\\.arclint$)", "(\\.json$)" ] }, "generated": { "type": "generated" }, "merge-conflict": { "type": "merge-conflict" }, "nolint": { "type": "nolint" }, "phutil-xhpast": { "type": "phutil-xhpast", "include": "(\\.php$)", "phutil-xhpast.deprecated.functions": { "phutil_escape_html": "The phutil_escape_html() function is deprecated. Raw strings passed to phutil_tag() or hsprintf() are escaped automatically." } }, "phutil-library": { "type": "phutil-library", "include": "(\\.php$)" }, "text": { "type": "text", "exclude": [ "(^\\.arclint)" ] }, "text-without-length": { "type": "text", "severity": { "3": "disabled" }, "include": [ "(^\\.arclint)" ] }, "spelling": { "type": "spelling" }, "xhpast": { "type": "xhpast", "include": "(\\.php$)", "severity": { "16": "advice", "34": "error" }, "xhpast.naminghook": "PhabricatorSymbolNameLinter" } } } diff --git a/src/docs/contributor/general_coding_standards.diviner b/src/docs/contributor/general_coding_standards.diviner index a2294d0e1..45081231b 100644 --- a/src/docs/contributor/general_coding_standards.diviner +++ b/src/docs/contributor/general_coding_standards.diviner @@ -1,147 +1,148 @@ @title General Coding Standards @group standards This document is a general coding standard for contributing to Phabricator, Arcanist, libphutil and Diviner. = Overview = This document contains practices and guidelines which apply across languages. Contributors should follow these guidelines. These guidelines are not hard-and-fast but should be followed unless there is a compelling reason to deviate from them. = Code Complexity = - Prefer to write simple code which is easy to understand. The simplest code is not necessarily the smallest, and some changes which make code larger (such as decomposing complex expressions and choosing more descriptive names) may also make it simpler. Be willing to make size tradeoffs in favor of simplicity. - Prefer simple methods and functions which take a small number of parameters. Avoid methods and functions which are long and complex, or take an innumerable host of parameters. When possible, decompose monolithic, complex methods into several focused, simpler ones. - Avoid putting many ideas on a single line of code. For example, avoid this kind of code: COUNTEREXAMPLE $category_map = array_combine( $dates, array_map(create_function('$z', 'return date("F Y", $z);'), $dates)); Expressing this complex transformation more simply produces more readable code: $category_map = array(); foreach ($dates as $date) { $category_map[$date] = date('F Y', $date); } And, obviously, don't do this sort of thing: COUNTEREXAMPLE if ($val = $some->complicatedConstruct() && !!~blarg_blarg_blarg() & $flags ? HOPE_YOU_MEMORIZED == $all_the_lexical_binding_powers : <<<'Q' ${hahaha} Q ); = Performance = - Prefer to write efficient code. - Strongly prefer to drive optimization decisions with hard data. Avoid optimizing based on intuition or rumor if you can not support it with concrete measurements. - Prefer to optimize code which is slow and runs often. Optimizing code which is fast and runs rarely is usually a waste of time, and can even be harmful if it makes that code more difficult to understand or maintain. You can determine if code is fast or slow by measuring it. - Reject performance discussions that aren't rooted in concrete data. In Phabricator, you can usually use the builtin XHProf profiling to quickly gather concrete performance data. = Naming Things = - Follow language-specific conventions. - Name things unambiguously. - Choose descriptive names. - - Avoid nonstandard abbreviations (common abbreviations like ID, URI and HTTP are fine). + - Avoid nonstandard abbreviations (common abbreviations like ID, URI and HTTP + are fine). - Spell words correctly. - Use correct grammar. For example, avoid these sorts of naming choices: COUNTEREXAMPLE $PIE->GET_FLAVOR(); // Unconventional. $thing->doStuff(); // Ambiguous. $list->empty(); // Ambiguous -- is it isEmpty() or makeEmpty()? $e = 3; // Not descriptive. $this->updtHndlr(); // Nonstandard abbreviation. $this->chackSpulls(); // Misspelling, ungrammatical. Prefer these: $pie->getFlavor(); // Conventional. $pie->bake(); // Unambiguous. $list->isEmpty(); // Unambiguous. $list->makeEmpty(); // Unambiguous. $edge_count = 3; // Descriptive. $this->updateHandler(); // No nonstandard abbreviations. $this->getID(); // Standard abbreviation. $this->checkSpelling(); // Correct spelling and grammar. = Error Handling = - Strongly prefer to detect errors. - Strongly prefer to fail fast and loudly. The maximum cost of script termination is known, bounded, and fairly small. The maximum cost of continuing script execution when errors have occurred is unknown and unbounded. This also makes APIs much easier to use and problems far easier to debug. When you ignore errors, defer error handling, or degrade the severity of errors by treating them as warnings and then dismissing them, you risk dangerous behavior which may be difficult to troubleshoot: COUNTEREXAMPLE exec('echo '.$data.' > file.bak'); // Bad! do_something_dangerous(); exec('echo '.$data.' > file.bak', $out, $err); // Also bad! if ($err) { debug_rlog("Unable to copy file!"); } do_something_dangerous(); Instead, fail loudly: exec('echo '.$data.' > file.bak', $out, $err); // Better if ($err) { throw new Exception("Unable to copy file!"); } do_something_dangerous(); But the best approach is to use or write an API which simplifies condition handling and makes it easier to get right than wrong: execx('echo %s > file.bak', $data); // Good do_something_dangerous(); Filesystem::writeFile('file.bak', $data); // Best do_something_dangerous(); See @{article@libphutil:Command Execution} for details on the APIs used in this example. = Documentation, Comments and Formatting = - Prefer to remove code by deleting it over removing it by commenting it out. It shall live forever in source control, and can be retrieved therefrom if it is ever again called upon. - In source code, use only ASCII printable characters plus space and linefeed. Do not use UTF-8 or other multibyte encodings. diff --git a/src/docs/contributor/php_coding_standards.diviner b/src/docs/contributor/php_coding_standards.diviner index 78021a2f9..93957845c 100644 --- a/src/docs/contributor/php_coding_standards.diviner +++ b/src/docs/contributor/php_coding_standards.diviner @@ -1,169 +1,170 @@ @title PHP Coding Standards @group standards -This document describes PHP coding standards for Phabricator and related projects (like Arcanist and libphutil). +This document describes PHP coding standards for Phabricator and related +projects (like Arcanist and libphutil). = Overview = This document outlines technical and style guidelines which are followed in libphutil. Contributors should also follow these guidelines. Many of these guidelines are automatically enforced by lint. These guidelines are essentially identical to the Facebook guidelines, since I basically copy-pasted them. If you are already familiar with the Facebook guidelines, you probably don't need to read this super thoroughly. = Spaces, Linebreaks and Indentation = - Use two spaces for indentation. Don't use tab literal characters. - Use Unix linebreaks ("\n"), not MSDOS ("\r\n") or OS9 ("\r"). - Use K&R style braces and spacing. - Put a space after control keywords like ##if## and ##for##. - Put a space after commas in argument lists. - Put a space around operators like ##=##, ##<##, etc. - Don't put spaces after function names. - Parentheses should hug their contents. - Generally, prefer to wrap code at 80 columns. = Case and Capitalization = - Name variables and functions using ##lowercase_with_underscores##. - Name classes using ##UpperCamelCase##. - Name methods and properties using ##lowerCamelCase##. - Use uppercase for common acronyms like ID and HTML. - Name constants using ##UPPERCASE##. - Write ##true##, ##false## and ##null## in lowercase. = Comments = - Do not use "#" (shell-style) comments. - Prefer "//" comments inside function and method bodies. = PHP Language Style = - Use "" tag. - Prefer casts like ##(string)## to casting functions like ##strval()##. - Prefer type checks like ##$v === null## to type functions like ##is_null()##. - Avoid all crazy alternate forms of language constructs like "endwhile" and "<>". - Always put braces around conditional and loop blocks. = PHP Language Features = - Use PHP as a programming language, not a templating language. - Avoid globals. - Avoid extract(). - Avoid eval(). - Avoid variable variables. - Prefer classes over functions. - Prefer class constants over defines. - Avoid naked class properties; instead, define accessors. - Use exceptions for error conditions. - Use type hints, use `assert_instances_of()` for arrays holding objects. = Examples = **if/else:** if ($some_variable > 3) { // ... } else if ($some_variable === null) { // ... } else { // ... } You should always put braces around the body of an if clause, even if it is only one line long. Note spaces around operators and after control statements. Do not use the "endif" construct, and write "else if" as two words. **for:** for ($ii = 0; $ii < 10; $ii++) { // ... } Prefer $ii, $jj, $kk, etc., as iterators, since they're easier to pick out visually and react better to "Find Next..." in editors. **foreach:** foreach ($map as $key => $value) { // ... } **switch:** switch ($value) { case 1: // ... break; case 2: if ($flag) { // ... break; } break; default: // ... break; } ##break## statements should be indented to block level. **array literals:** $junk = array( 'nuts', 'bolts', 'refuse', ); Use a trailing comma and put the closing parenthesis on a separate line so that diffs which add elements to the array affect only one line. **operators:** $a + $b; // Put spaces around operators. $omg.$lol; // Exception: no spaces around string concatenation. $arr[] = $element; // Couple [] with the array when appending. $obj = new Thing(); // Always use parens. **function/method calls:** // One line eject($cargo); // Multiline AbstractFireFactoryFactoryEngine::promulgateConflagrationInstance( $fuel, $ignition_source); **function/method definitions:** function example_function($base_value, $additional_value) { return $base_value + $additional_value; } class C { public static function promulgateConflagrationInstance( IFuel $fuel, IgnitionSource $source) { // ... } } **class:** class Dog extends Animal { const CIRCLES_REQUIRED_TO_LIE_DOWN = 3; private $favoriteFood = 'dirt'; public function getFavoriteFood() { return $this->favoriteFood; } } diff --git a/src/docs/flavor/writing_reviewable_code.diviner b/src/docs/flavor/writing_reviewable_code.diviner index 7e840b7b6..955fe6590 100644 --- a/src/docs/flavor/writing_reviewable_code.diviner +++ b/src/docs/flavor/writing_reviewable_code.diviner @@ -1,163 +1,163 @@ @title Writing Reviewable Code @group review Project recommendations on how to structure changes. This document is purely advisory. Phabricator works with a variety of revision control strategies, and diverging from the recommendations in this document will not impact your ability to use it for code review and source management. = Overview = This document describes a strategy for structuring changes used successfully at Facebook and in Phabricator. In essence: - Each commit should be as small as possible, but no smaller. - The smallest a commit can be is a single cohesive idea: don't make commits so small that they are meaningless on their own. - There should be a one-to-one mapping between ideas and commits: each commit should build one idea, and each idea should be implemented by one commit. - Turn large commits into small commits by dividing large problems into smaller problems and solving the small problems one at a time. - Write sensible commit messages. = Many Small Commits = Small, simple commits are generally better than large, complex commits. They are easier to understand, easier to test, and easier to review. The complexity of understanding, testing and reviewing a change often increases faster than its size: ten 200-line changes each doing one thing are often far easier to understand than one 2,000 line change doing ten things. Splitting a change which does many things into smaller changes which each do only one thing can decrease the total complexity associated with accomplishing the same goal. Each commit should do one thing. Generally, this means that you should separate distinct changes into different commits when developing. For example, if you're developing a feature and run into a preexisting bug, stash or checkpoint your change, check out a clean HEAD/tip, fix the bug in one change, and then merge/rebase your new feature on top of your bugfix so that you have two changes, each with one idea ("add feature x", "fix a bug in y"), not one change with two ideas ("add feature x and fix a bug in y"). (In Git, you can do this easily with local feature branches and commands like `git rebase`, `git rebase -i`, and `git stash`, or with merges. In Mercurial, you can use bookmarks or the queues extension. In SVN, there are few builtin tools, but you can use multiple working copies or treat Differential like a stash you access with `arc patch`.) Even changes like fixing style problems should ideally be separated: they're accomplishing a different goal. And it is far easier to review one 300-line change which "converts tabs to spaces" plus one 30-line change which "implements feature z" than one 330-line change which "implements feature z and also converts a bunch of tabs to spaces". Similarly, break related but complex changes into smaller, simpler components. Here's a ridiculous analogy: if you're adding a new house, don't make one 5,000-line change which adds the whole house in one fell sweep. Split it apart into smaller steps which are each easy to understand: start with the foundation, then build the frame, etc. If you decided to dig the foundation with a shovel or build the frame out of cardboard, it's both easier to miss and harder to correct if the decisions are buried in 5,000 lines of interior design and landscaping. Do it one piece at a time, providing enough context that the larger problem can be understood but accomplishing no more with each step than you need to in order for it to stand on its own. The minimum size of a change should be a complete implementation of the simplest subproblem which works on its own and expresses an entire idea, not just part of an idea. You could mechanically split a 1,000-line change into ten 100-line changes by choosing lines at random, but none of the individual changes would make any sense and you would increase the collective complexity. The real goal is for each change to have minimal complexity, line size is just a proxy that is often well-correlated with complexity. We generally follow these practices in Phabricator. The median change size for Phabricator is 35 lines. See @{article:Differential User Guide: Large Changes} for information about reviewing big checkins. = Write Sensible Commit Messages = -There are lots of resources for this on the internet. All of them say pretty much -the same thing; this one does too. +There are lots of resources for this on the internet. All of them say pretty +much the same thing; this one does too. The single most important thing is: **commit messages should explain //why// you are making the change**. Differential attempts to encourage the construction of sensible commit messages, but can only enforce structure, not content. Structurally, commit messages should probably: - Have a title, briefly describing the change in one line. - Have a summary, describing the change in more detail. - Maybe have some other fields. The content is far more important than the structure. In particular, the summary should explain //why// you're making the change and //why// you're choosing the implementation you're choosing. The //what// of the change is generally well-explained by the change itself. For example, this is obviously an awful commit message: COUNTEREXAMPLE fix a bug But this one is almost as bad: COUNTEREXAMPLE Allow dots in usernames Change the regexps so usernames can have dots in them. This is better than nothing but just summarizes information which can be inferred from the text of the diff. Instead, you should provide context and explain why you're making the change you're making, and why it's the right one: lang=txt Allow dots in usernames to support Google and LDAP auth To prevent nonsense, usernames are currently restricted to A-Z0-9. Now that we have Google and LDAP auth, a couple of installs want to allow "." too since they have schemes like "abraham.lincoln@mycompany.com" (see Tnnn). There are no technical reasons not to do this, so I opened up the regexps a bit. We could mostly open this up more but I figured I'd wait until someone asks before allowing "ke$ha", etc., because I personally find such names distasteful and offensive. This information can not be extracted from the change itself, and is much more useful for the reviewer and for anyone trying to understand the change after the fact. An easy way to explain //why// is to reference other objects (bugs/issues/revisions) which motivate the change. Differential also includes a "Test Plan" field which is required by default. There is a detailed description of this field in @{article:Differential User Guide: Test Plans}. You can make it optional or disable it in the configuration, but consider adopting it. Having this information can be particularly helpful for reviewers. Some things that people sometimes feel strongly about but which are probably not really all that important in commit messages include: - If/where text is wrapped. - Maximum length of the title. - Whether there should be a period or not in the title. - Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds". - Other sorts of pedantry not related to getting the context and reasons //why// a change is happening into the commit message. - Although maybe the spelling and grammar shouldn't be egregiously bad? Phabricator does not have guidelines for this stuff. You can obviously set guidelines at your organization if you prefer, but getting the //why// into the message is the most important part. = Next Steps = Continue by: - reading recommendations on structuring revision control with @{article:Recommendations on Revision Control}; or - reading recommendations on structuring branches with @{article:Recommendations on Branching}. diff --git a/src/docs/user/userguide/arcanist_lint_unit.diviner b/src/docs/user/userguide/arcanist_lint_unit.diviner index 1596a0bbe..d9cc10744 100644 --- a/src/docs/user/userguide/arcanist_lint_unit.diviner +++ b/src/docs/user/userguide/arcanist_lint_unit.diviner @@ -1,91 +1,92 @@ @title Arcanist User Guide: Customizing Lint, Unit Tests and Workflows @group userguide Explains how to build new classes to control how Arcanist behaves. This is a configuration guide that helps you set up advanced features. If you're just getting started, you don't need to look at this yet. Instead, start with the @{article:Arcanist User Guide}. = Overview = Arcanist has some basic configuration options available in the `.arcconfig` file (see @{article:Arcanist User Guide: Configuring a New Project}), but it can't handle everything. If you want to customize Arcanist at a deeper level, you need to build new classes. For instance: - if you want to configure linters, or add new linters, you need to create a new class which extends @{class@arcanist:ArcanistLintEngine}. - if you want to integrate with a unit testing framework, you need to create a new class which extends @{class@arcanist:ArcanistBaseUnitTestEngine}. - if you you want to change how workflows behave, or add new workflows, you need to create a new class which extends @{class@arcanist:ArcanistConfiguration}. Arcanist works through a sort of dependency-injection approach. For example, Arcanist does not run lint rules by default, but you can set `lint.engine` in your `.arcconfig` to the name of a class which extends @{class@arcanist:ArcanistLintEngine}. When running from inside your project, Arcanist will load this class and call methods on it in order to run lint. To make this work, you need to do three things: - actually write the class; - add the library where the class exists to your ##.arcconfig##; - add the class name to your ##.arcconfig## as the **lint.engine**, **unit.engine**, or **arcanist_configuration**. = Create a libphutil Library = If you haven't created a library for the class to live in yet, you need to do -that first. Follow the instructions in @{article:libphutil Libraries User Guide}, -then make the library loadable by adding it to your ##.arcconfig## like this: +that first. Follow the instructions in +@{article:libphutil Libraries User Guide}, then make the library loadable by +adding it to your ##.arcconfig## like this: { // ... "load" : [ // ... "/path/to/my/library", // Absolute path "support/arcanist", // Relative path in this project // ... ] // ... } You can either specify an absolute path, or a path relative to the project root. When you run ##arc list --trace##, you should see a message to the effect that it has loaded your library. For debugging or testing, you can also run Arcanist with the ##--load-phutil-library## flag: arc --load-phutil-library=/path/to/library You can specify this flag more than once to load several libraries. Note that if you use this flag, Arcanist will ignore any libraries listed in ##.arcconfig##. = Use the Class = This step is easy: just edit ##.arcconfig## to specify your class name as the appropriate configuration value. { // ... "lint.engine" : "CustomArcanistLintEngine", // ... } Now, when you run Arcanist in your project, it will invoke your class when appropriate. For lint and unit tests, you can also use the ##--engine## flag override the default engine: arc lint --engine MyCustomArcanistLintEngine This is mostly useful for debugging and testing. = Next Steps = - Learn how to reuse existing linters by reading @{article:Arcanist User Guide: Customizing Existing Linters}. diff --git a/src/docs/user/userguide/diviner.diviner b/src/docs/user/userguide/diviner.diviner index e392a606d..e94c33d27 100644 --- a/src/docs/user/userguide/diviner.diviner +++ b/src/docs/user/userguide/diviner.diviner @@ -1,84 +1,84 @@ @title Diviner User Guide @group userguide Using Diviner, a documentation generator. = Overview = NOTE: Diviner is new and not yet generally useful. = Generating Documentation = To generate documentation, run: phabricator/ $ ./bin/diviner generate --book = .book Files = Diviner documentation books are configured using JSON `.book` files, which look like this: name=example.book { "name" : "example", "title" : "Example Documentation", "short" : "Example Docs", "root" : ".", "uri.source" : "http://example.com/diffusion/X/browse/master/%f$%l", "rules" : { "(\\.diviner$)" : "DivinerArticleAtomizer" }, "exclude" : [ "(^externals/)", "(^scripts/)", "(^support/)" ], "groups" : { "forward" : { "name" : "Doing Stuff" }, "reverse" : { "name" : "Undoing Stuff" } } } The properties in this file are: - - `name`: Required. Short, unique name to identify the documentation book. This - will be used in URIs, so it should not have special characters. Good names - are things like `"example"` or `"libcabin"`. + - `name`: Required. Short, unique name to identify the documentation book. + This will be used in URIs, so it should not have special characters. Good + names are things like `"example"` or `"libcabin"`. - `root`: Required. The root directory (relative to the `.book` file) which documentation should be generated from. Often this will be a value like `"../../"`, to specify the project root (for example, if the `.book` file is in `project/src/docs/example.book`, the value `"../../"` would generate documentation from the `project/` directory. - `title`: Optional. Full human-readable title of the documentation book. This is used when there's plenty of display space and should completely describe the book. Good titles are things like `"Example Documentation"`, or `"libcabin Developer Documentation"`. - `short`: Optional. Shorter version of the title for use when display space is limited (for example, in navigation breadcrumbs). If omitted, the full title is used. Good short titles are things like `"Example Docs"` or `"libcabin Dev Docs"`. - `uri.source`: Optional. Diviner can link from the documentation to a repository browser so that you can quickly jump to the definition of a class or function. To do this, it uses a URI pattern which you specify here. Normally, this URI should point at a repository browser like Diffusion. For example, `"http://repobrowser.yourcompany.com/%f#%l"`. You can use these conversions in the URI, which will be replaced at runtime: - `%f`: Replaced with the name of the file. - `%l`: Replaced with the line number. - `%%`: Replaced with a literal `%` symbol. - `rules`: Optional. A map of regular expressions to Atomizer classes which controls which documentation generator runs on each file. If omitted, Diviner will use its default ruleset. For example, adding the key `"(\\.diviner$)"` to the map with value `"DivinerArticleAtomizer"` tells Diviner to analyze any file with a name ending in `.diviner` using the "article" atomizer. - `exclude`: Optional. A list of regular expressions matching paths which will be excluded from documentation generation for this book. For example, adding a pattern like `"(^externals/)"` or `"(^vendor/)"` will make Diviner ignore those directories. - `groups`: Optional. Describes top level organizational groups which atoms should be placed into.