Page MenuHomestyx hydra
Files F233307

No OneTemporary
Actions

View Options

diff --git a/src/docs/book/contributor.book b/src/docs/book/contributor.book
new file mode 100644
index 0000000000..2960d4e439
--- /dev/null
+++ b/src/docs/book/contributor.book
@@ -0,0 +1,33 @@
+{
+ "name" : "phabcontrib",
+ "title" : "Phabricator Contributor Documentation",
+ "short" : "Phabricator Contributor Docs",
+ "preface" : "Information for Phabricator contributors.",
+ "root" : "../../../",
+ "uri.source" :
+ "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l",
+ "rules" : {
+ "(\\.diviner$)" : "DivinerArticleAtomizer"
+ },
+ "exclude" : [
+ "(^externals/)",
+ "(^webroot/rsrc/externals/)",
+ "(^scripts/)",
+ "(^support/)",
+ "(^resources/)",
+ "(^src/docs/user/)",
+ "(^src/docs/tech/)",
+ "(^src/docs/flavor/)"
+ ],
+ "groups" : {
+ "contrib" : {
+ "name" : "Contributor Overview"
+ },
+ "standards" : {
+ "name" : "Coding Standards"
+ },
+ "developer" : {
+ "name" : "Developer Guides"
+ }
+ }
+}
diff --git a/src/docs/book/flavor.book b/src/docs/book/flavor.book
new file mode 100644
index 0000000000..92e5158ece
--- /dev/null
+++ b/src/docs/book/flavor.book
@@ -0,0 +1,42 @@
+{
+ "name" : "phabflavor",
+ "title" : "Phabricator Flavor Text",
+ "short" : "Flavor Text",
+ "preface" : "Recommendations, lore, and dark rituals.",
+ "root" : "../../../",
+ "uri.source" :
+ "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l",
+ "rules" : {
+ "(\\.diviner$)" : "DivinerArticleAtomizer"
+ },
+ "exclude" : [
+ "(^externals/)",
+ "(^webroot/rsrc/externals/)",
+ "(^scripts/)",
+ "(^support/)",
+ "(^resources/)",
+ "(^src/docs/user/)",
+ "(^src/docs/tech/)",
+ "(^src/docs/contributor/)"
+ ],
+ "groups" : {
+ "overview" : {
+ "name" : "Overview"
+ },
+ "review" : {
+ "name" : "Revision Control and Code Review"
+ },
+ "sundry" : {
+ "name" : "Sundries"
+ },
+ "lore" : {
+ "name" : "Phabricator Lore"
+ },
+ "php" : {
+ "name" : "PHP"
+ },
+ "javascript" : {
+ "name" : "Javascript"
+ }
+ }
+}
diff --git a/src/docs/book/phabricator.book b/src/docs/book/phabricator.book
index 7b8c794d6b..cf363a647d 100644
--- a/src/docs/book/phabricator.book
+++ b/src/docs/book/phabricator.book
@@ -1,271 +1,273 @@
{
"name" : "phabdev",
"title" : "Phabricator Technical Documentation",
"short" : "Phabricator Tech Docs",
"preface" : "Technical documentation intended for Phabricator developers.",
"root" : "../../../",
"uri.source" :
"https://secure.phabricator.com/diffusion/P/browse/master/%f$%l",
"rules" : {
"(\\.php$)" : "DivinerPHPAtomizer",
"(\\.diviner$)" : "DivinerArticleAtomizer"
},
"exclude" : [
"(^externals/)",
"(^webroot/rsrc/externals/)",
"(^scripts/)",
"(^support/)",
"(^resources/)",
- "(^src/docs/user/)"
+ "(^src/docs/user/)",
+ "(^src/docs/flavor/)",
+ "(^src/docs/contributor/)"
],
"groups" : {
"arcanist" : {
"name" : "Arcanist Integration",
"include" : "(^src/applications/arcanist/)"
},
"audit" : {
"name" : "Audit",
"include" : "(^src/applications/audit/)"
},
"auth" : {
"name" : "Auth",
"include" : "(^src/applications/auth/)"
},
"baseapp" : {
"name" : "Application Basics",
"include" : "(^src/applications/base/)"
},
"cache" : {
"name" : "Cache",
"include" : "(^src/applications/cache/)"
},
"calendar" : {
"name" : "Calendar",
"include" : "(^src/applications/calendar/)"
},
"chatlog" : {
"name" : "Chatlog",
"include" : "(^src/applications/chatlog/)"
},
"conduit" : {
"name" : "Conduit",
"include" : "(^src/applications/conduit/)"
},
"config" : {
"name" : "Config",
"include" : "(^src/applications/config/)"
},
"conpherence" : {
"name" : "Conpherence",
"include" : "(^src/applications/conpherence/)"
},
"countdown" : {
"name" : "Countdown",
"include" : "(^src/applications/countdown/)"
},
"daemon" : {
"name" : "Daemons",
"include" : "(^src/applications/daemon/)"
},
"differential" : {
"name" : "Differential",
"include" : "(^src/applications/differential/)"
},
"diffusion" : {
"name" : "Diffusion",
"include" : "(^src/applications/diffusion/)"
},
"directory" : {
"name" : "Directory",
"include" : "(^src/applications/directory/)"
},
"diviner" : {
"name" : "Diviner",
"include" : "(^src/applications/diviner/)"
},
"doorkeeper" : {
"name" : "Doorkeeper",
"include" : "(^src/applications/doorkeeper/)"
},
"draft" : {
"name" : "Draft",
"include" : "(^src/applications/draft/)"
},
"drydock" : {
"name" : "Drydock",
"include" : "(^src/applications/drydock/)"
},
"fact" : {
"name" : "Fact",
"include" : "(^src/applications/fact/)"
},
"feed" : {
"name" : "Feed",
"include" : "(^src/applications/feed/)"
},
"files" : {
"name" : "Files",
"include" : "(^src/applications/files/)"
},
"flag" : {
"name" : "Flags",
"include" : "(^src/applications/flag/)"
},
"harbormaster" : {
"name" : "Harbormaster",
"include" : "(^src/applications/harbormaster/)"
},
"help" : {
"name" : "Help",
"include" : "(^src/applications/help/)"
},
"herald" : {
"name" : "Herald",
"include" : "(^src/applications/herald/)"
},
"legalpad" : {
"name" : "Legalpad",
"include" : "(^src/applications/legalpad/)"
},
"lipsum" : {
"name" : "Lipsum",
"include" : "(^src/applications/lipsum/)"
},
"macro" : {
"name" : "Macro",
"include" : "(^src/applications/macro/)"
},
"mailinglists" : {
"name" : "Mailing Lists",
"include" : "(^src/applications/mailinglists/)"
},
"maniphest" : {
"name" : "Maniphest",
"include" : "(^src/applications/maniphest/)"
},
"meta" : {
"name" : "Meta",
"include" : "(^src/applications/meta/)"
},
"metamta" : {
"name" : "MetaMTA",
"include" : "(^src/applications/metamta/)"
},
"notification" : {
"name" : "Notifications",
"include" : "(^src/applications/notification/)"
},
"oauthserver" : {
"name" : "OAuth Server",
"include" : "(^src/applications/oauthserver/)"
},
"owners" : {
"name" : "Owners",
"include" : "(^src/applications/owners/)"
},
"paste" : {
"name" : "Paste",
"include" : "(^src/applications/paste/)"
},
"people" : {
"name" : "People",
"include" : "(^src/applications/people/)"
},
"phame" : {
"name" : "Phame",
"include" : "(^src/applications/phame/)"
},
"phid" : {
"name" : "PHIDs",
"include" : "(^src/applications/phid/)"
},
"phlux" : {
"name" : "Phlux",
"include" : "(^src/applications/phlux/)"
},
"pholio" : {
"name" : "Pholio",
"include" : "(^src/applications/pholio/)"
},
"phortune" : {
"name" : "Phortune",
"include" : "(^src/applications/phortune/)"
},
"phpast" : {
"name" : "PHPAST",
"include" : "(^src/applications/phpast/)"
},
"phrequent" : {
"name" : "Phrequent",
"include" : "(^src/applications/phrequent/)"
},
"phriction" : {
"name" : "Phriction",
"include" : "(^src/applications/phriction/)"
},
"policy" : {
"name" : "Policy",
"include" : "(^src/applications/policy/)"
},
"ponder" : {
"name" : "Ponder",
"include" : "(^src/applications/ponder/)"
},
"project" : {
"name" : "Projects",
"include" : "(^src/applications/project/)"
},
"releeph" : {
"name" : "Releeph",
"include" : "(^src/applications/releeph/)"
},
"remarkup" : {
"name" : "Remarkup",
"include" : "(^src/applications/remarkup/)"
},
"repository" : {
"name" : "Repositories",
"include" : "(^src/applications/repository/)"
},
"search" : {
"name" : "Search",
"include" : "(^src/applications/search/)"
},
"settings" : {
"name" : "Settings",
"include" : "(^src/applications/settings/)"
},
"slowvote" : {
"name" : "Slowvote",
"include" : "(^src/applications/slowvote/)"
},
"subscriptions" : {
"name" : "Subscriptions",
"include" : "(^src/applications/subscriptions/)"
},
"system" : {
"name" : "System",
"include" : "(^src/applications/system/)"
},
"tokens" : {
"name" : "Tokens",
"include" : "(^src/applications/tokens/)"
},
"transactions" : {
"name" : "Transactions",
"include" : "(^src/applications/transactions/)"
},
"typeahead" : {
"name" : "Typeahead",
"include" : "(^src/applications/typeahead/)"
},
"uiexample" : {
"name" : "UI Examples",
"include" : "(^src/applications/uiexample/)"
},
"xhprof" : {
"name" : "XHProf",
"include" : "(^src/applications/xhprof/)"
}
}
}
diff --git a/src/docs/book/user.book b/src/docs/book/user.book
index 96c7df40a2..e05fb21f20 100644
--- a/src/docs/book/user.book
+++ b/src/docs/book/user.book
@@ -1,49 +1,51 @@
{
"name" : "phabricator",
"title" : "Phabricator User Documentation",
"short" : "Phabricator User Docs",
"preface" : "Instructions for installing, configuring, and using Phabricator.",
"root" : "../../../",
"uri.source" :
"https://secure.phabricator.com/diffusion/P/browse/master/%f$%l",
"rules" : {
"(\\.diviner$)" : "DivinerArticleAtomizer"
},
"exclude" : [
"(^externals/)",
"(^webroot/rsrc/externals/)",
"(^scripts/)",
"(^support/)",
"(^resources/)",
- "(^src/docs/tech/)"
+ "(^src/docs/tech/)",
+ "(^src/docs/flavor/)",
+ "(^src/docs/contributor/)"
],
"groups" : {
"intro" : {
"name" : "Introduction"
},
"config" : {
"name" : "Configuration"
},
"userguide" : {
"name" : "Application User Guides"
},
"differential" : {
"name" : "Differential (Code Review)"
},
"diffusion" : {
"name" : "Diffusion (Repository Browser)"
},
"maniphest" : {
"name" : "Maniphest (Task Tracking)"
},
"slowvote" : {
"name" : "Slowvote (Polls)"
},
"herald" : {
"name" : "Herald (Notifications)"
},
"phriction" : {
"name" : "Phriction (Wiki)"
}
}
}
diff --git a/src/docs/user/developer/adding_new_css_and_js.diviner b/src/docs/contributor/adding_new_css_and_js.diviner
similarity index 100%
rename from src/docs/user/developer/adding_new_css_and_js.diviner
rename to src/docs/contributor/adding_new_css_and_js.diviner
diff --git a/src/docs/user/contributing/contrib_intro.diviner b/src/docs/contributor/contrib_intro.diviner
similarity index 100%
rename from src/docs/user/contributing/contrib_intro.diviner
rename to src/docs/contributor/contrib_intro.diviner
diff --git a/src/docs/user/developer/darkconsole.diviner b/src/docs/contributor/darkconsole.diviner
similarity index 100%
rename from src/docs/user/developer/darkconsole.diviner
rename to src/docs/contributor/darkconsole.diviner
diff --git a/src/docs/user/developer/database.diviner b/src/docs/contributor/database.diviner
similarity index 100%
rename from src/docs/user/developer/database.diviner
rename to src/docs/contributor/database.diviner
diff --git a/src/docs/user/contributing/general_coding_standards.diviner b/src/docs/contributor/general_coding_standards.diviner
similarity index 99%
rename from src/docs/user/contributing/general_coding_standards.diviner
rename to src/docs/contributor/general_coding_standards.diviner
index 0ed00f98d3..a2294d0e10 100644
--- a/src/docs/user/contributing/general_coding_standards.diviner
+++ b/src/docs/contributor/general_coding_standards.diviner
@@ -1,147 +1,147 @@
@title General Coding Standards
-@group contrib
+@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).
- 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/user/contributing/internationalization.diviner b/src/docs/contributor/internationalization.diviner
similarity index 99%
rename from src/docs/user/contributing/internationalization.diviner
rename to src/docs/contributor/internationalization.diviner
index 56f7ac221d..25d790e808 100644
--- a/src/docs/user/contributing/internationalization.diviner
+++ b/src/docs/contributor/internationalization.diviner
@@ -1,64 +1,64 @@
@title Internationalization
-@group contrib
+@group developer
What is required from developers to get Phabricator translatable.
= API =
Translator API is provided by libphutil. It gives us
@{class@libphutil:PhutilTranslator} class and global @{function@libphutil:pht}
function built on top of it.
Developers are supposed to call @{function@libphutil:pht} on all strings that
require translation.
Phabricator provides translations for this translator through
@{class:PhabricatorTranslation} class.
= Adding a New Translation =
Adding a translation which uses the same language rules as some already existing
translation is relatively simple: Just extend @{class:PhabricatorTranslation}
and you will be able to specify this class in the global configuration
'translation.provider' and users will be able to select it in their preferences.
= Adding a New Language =
Adding a language involves all steps as adding a translation plus specifying the
language rules in @{method@libphutil:PhutilTranslator::chooseVariant}.
= Singular and Plural =
Different languages have various rules for using singular and plural. All you
need to do is to call @{function@libphutil:pht} with a text that is suitable for
both forms. Example:
pht('%d beer(s)', $count);
Translators will translate this text for all different forms the language uses:
// English translation
array('%d beer', '%d beers');
// Czech translation
array('%d pivo', '%d piva', '%d piv');
The ugly identifier passed to @{function@libphutil:pht} will remain in the text
only if the translation doesn't exist.
= Male and Female =
Different languages use different words for talking about males, females and
unknown genders. Callsites have to call @{function@libphutil:pht} passing
@{class:PhabricatorUser} (or other implementation of
@{interface@libphutil:PhutilPerson}) if talking about the user. Example:
pht('%s wrote', $actor);
Translators will create this translations:
// English translation
'%s wrote';
// Czech translation
array('%s napsal', '%s napsala');
diff --git a/src/docs/user/contributing/javascript_coding_standards.diviner b/src/docs/contributor/javascript_coding_standards.diviner
similarity index 99%
rename from src/docs/user/contributing/javascript_coding_standards.diviner
rename to src/docs/contributor/javascript_coding_standards.diviner
index 8634e39caf..83772918c3 100644
--- a/src/docs/user/contributing/javascript_coding_standards.diviner
+++ b/src/docs/contributor/javascript_coding_standards.diviner
@@ -1,140 +1,140 @@
@title Javascript Coding Standards
-@group contrib
+@group standards
This document describes Javascript coding standards for Phabricator and Javelin.
= Overview =
This document outlines technical and style guidelines which are followed in
Phabricator and Javelin. 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 can probably get away with skimming this document.
= Spaces, Linebreaks and Indentation =
- Use two spaces for indentation. Don't use literal tab 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 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 =
The Javascript language unambiguously dictates casing/naming rules; follow those
rules.
- Name variables using ##lowercase_with_underscores##.
- Name classes using ##UpperCamelCase##.
- Name methods and properties using ##lowerCamelCase##.
- Name global functions using ##lowerCamelCase##. Avoid defining global
functions.
- Name constants using ##UPPERCASE##.
- Write ##true##, ##false##, and ##null## in lowercase.
- "Internal" methods and properties should be prefixed with an underscore.
For more information about what "internal" means, see
**Leading Underscores**, below.
= Comments =
- Strongly prefer ##//## comments for making comments inside the bodies of
functions and methods (this lets someone easily comment out a block of code
while debugging later).
= Javascript Language =
- Use ##[]## and ##{}##, not ##new Array## and ##new Object##.
- When creating an object literal, do not quote keys unless required.
= Examples =
**if/else:**
lang=js
if (x > 3) {
// ...
} else if (x === null) {
// ...
} else {
// ...
}
You should always put braces around the body of an if clause, even if it is only
one line. Note that operators like ##>## and ##===## are also surrounded by
spaces.
**for (iteration):**
lang=js
for (var 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.
**for (enumeration):**
lang=js
for (var k in obj) {
// ...
}
Make sure you use enumeration only on Objects, not on Arrays. For more details,
see @{article:Javascript Object and Array}.
**switch:**
lang=js
switch (x) {
case 1:
// ...
break;
case 2:
if (flag) {
break;
}
break;
default:
// ...
break;
}
##break## statements should be indented to block level. If you don't push them
in, you end up with an inconsistent rule for conditional ##break## statements,
as in the ##2## case.
If you insist on having a "fall through" case that does not end with ##break##,
make it clear in a comment that you wrote this intentionally. For instance:
lang=js
switch (x) {
case 1:
// ...
// Fall through...
case 2:
//...
break;
}
= Leading Underscores =
By convention, methods names which start with a leading underscore are
considered "internal", which (roughly) means "private". The critical difference
is that this is treated as a signal to Javascript processing scripts that a
symbol is safe to rename since it is not referenced outside the current file.
The upshot here is:
- name internal methods which shouldn't be called outside of a file's scope
with a leading underscore; and
- **never** call an internal method from another file.
If you treat them as though they were "private", you won't run into problems.
diff --git a/src/docs/user/developer/n_plus_one.diviner b/src/docs/contributor/n_plus_one.diviner
similarity index 100%
rename from src/docs/user/developer/n_plus_one.diviner
rename to src/docs/contributor/n_plus_one.diviner
diff --git a/src/docs/user/developer/phabricator_code_layout.diviner b/src/docs/contributor/phabricator_code_layout.diviner
similarity index 100%
rename from src/docs/user/developer/phabricator_code_layout.diviner
rename to src/docs/contributor/phabricator_code_layout.diviner
diff --git a/src/docs/user/contributing/php_coding_standards.diviner b/src/docs/contributor/php_coding_standards.diviner
similarity index 99%
rename from src/docs/user/contributing/php_coding_standards.diviner
rename to src/docs/contributor/php_coding_standards.diviner
index cb3406e15b..78021a2f9f 100644
--- a/src/docs/user/contributing/php_coding_standards.diviner
+++ b/src/docs/contributor/php_coding_standards.diviner
@@ -1,169 +1,169 @@
@title PHP Coding Standards
-@group contrib
+@group standards
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 "<?php", not the "<?" short form. Omit the closing "?>" 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/user/developer/rendering_html.diviner b/src/docs/contributor/rendering_html.diviner
similarity index 100%
rename from src/docs/user/developer/rendering_html.diviner
rename to src/docs/contributor/rendering_html.diviner
diff --git a/src/docs/user/developer/running_builtin_php_webserver.diviner b/src/docs/contributor/running_builtin_php_webserver.diviner
similarity index 100%
rename from src/docs/user/developer/running_builtin_php_webserver.diviner
rename to src/docs/contributor/running_builtin_php_webserver.diviner
diff --git a/src/docs/user/developer/unit_tests.diviner b/src/docs/contributor/unit_tests.diviner
similarity index 100%
rename from src/docs/user/developer/unit_tests.diviner
rename to src/docs/contributor/unit_tests.diviner
diff --git a/src/docs/user/developer/using_edges.diviner b/src/docs/contributor/using_edges.diviner
similarity index 100%
rename from src/docs/user/developer/using_edges.diviner
rename to src/docs/contributor/using_edges.diviner
diff --git a/src/docs/user/developer/using_oauthserver.diviner b/src/docs/contributor/using_oauthserver.diviner
similarity index 100%
rename from src/docs/user/developer/using_oauthserver.diviner
rename to src/docs/contributor/using_oauthserver.diviner
diff --git a/src/docs/user/flavortext/about_flavor_text.diviner b/src/docs/flavor/about_flavor_text.diviner
similarity index 92%
rename from src/docs/user/flavortext/about_flavor_text.diviner
rename to src/docs/flavor/about_flavor_text.diviner
index 17cc8d317b..3d46ff41d0 100644
--- a/src/docs/user/flavortext/about_flavor_text.diviner
+++ b/src/docs/flavor/about_flavor_text.diviner
@@ -1,9 +1,9 @@
@title About Flavor Text
-@group flavortext
+@group overview
Explains what's going on here.
= Overview =
Flavor Text is a collection of short articles which pertain to software
development in general, not necessarily to Phabricator specifically.
diff --git a/src/docs/user/flavortext/javascript_object_array.diviner b/src/docs/flavor/javascript_object_array.diviner
similarity index 99%
rename from src/docs/user/flavortext/javascript_object_array.diviner
rename to src/docs/flavor/javascript_object_array.diviner
index fde8b33ecf..cf7e3371e6 100644
--- a/src/docs/user/flavortext/javascript_object_array.diviner
+++ b/src/docs/flavor/javascript_object_array.diviner
@@ -1,152 +1,152 @@
@title Javascript Object and Array
-@group flavortext
+@group javascript
This document describes the behaviors of Object and Array in Javascript, and
a specific approach to their use which produces basically reasonable language
behavior.
= Primitives =
Javascript has two native datatype primitives, Object and Array. Both are
classes, so you can use ##new## to instantiate new objects and arrays:
COUNTEREXAMPLE
var a = new Array(); // Not preferred.
var o = new Object();
However, **you should prefer the shorthand notation** because it's more concise:
lang=js
var a = []; // Preferred.
var o = {};
(A possible exception to this rule is if you want to use the allocation behavior
of the Array constructor, but you almost certainly don't.)
The language relationship between Object and Array is somewhat tricky. Object
and Array are both classes, but "object" is also a primitive type. Object is
//also// the base class of all classes.
lang=js
typeof Object; // "function"
typeof Array; // "function"
typeof {}; // "object"
typeof []; // "object"
var a = [], o = {};
o instanceof Object; // true
o instanceof Array; // false
a instanceof Object; // true
a instanceof Array; // true
= Objects are Maps, Arrays are Lists =
PHP has a single ##array## datatype which behaves like as both map and a list,
and a common mistake is to treat Javascript arrays (or objects) in the same way.
**Don't do this.** It sort of works until it doesn't. Instead, learn how
Javascript's native datatypes work and use them properly.
In Javascript, you should think of Objects as maps ("dictionaries") and Arrays
as lists ("vectors").
You store keys-value pairs in a map, and store ordered values in a list. So,
store key-value pairs in Objects.
var o = { // Good, an object is a map.
name: 'Hubert',
species: 'zebra'
};
console.log(o.name);
...and store ordered values in Arrays.
var a = [1, 2, 3]; // Good, an array is a list.
a.push(4);
Don't store key-value pairs in Arrays and don't expect Objects to be ordered.
COUNTEREXAMPLE
var a = [];
a['name'] = 'Hubert'; // No! Don't do this!
This technically works because Arrays are Objects and you think everything is
fine and dandy, but it won't do what you want and will burn you.
= Iterating over Maps and Lists =
Iterate over a map like this:
lang=js
for (var k in object) {
f(object[k]);
}
NOTE: There's some hasOwnProperty nonsense being omitted here, see below.
Iterate over a list like this:
lang=js
for (var ii = 0; ii < list.length; ii++) {
f(list[ii]);
}
NOTE: There's some sparse array nonsense being omitted here, see below.
If you try to use ##for (var k in ...)## syntax to iterate over an Array, you'll
pick up a whole pile of keys you didn't intend to and it won't work. If you try
to use ##for (var ii = 0; ...)## syntax to iterate over an Object, it won't work
at all.
If you consistently treat Arrays as lists and Objects as maps and use the
corresponding iterators, everything will pretty much always work in a reasonable
way.
= hasOwnProperty() =
An issue with this model is that if you write stuff to Object.prototype, it will
show up every time you use enumeration ##for##:
COUNTEREXAMPLE
var o = {};
Object.prototype.duck = "quack";
for (var k in o) {
console.log(o[k]); // Logs "quack"
}
There are two ways to avoid this:
- test that ##k## exists on ##o## by calling ##o.hasOwnProperty(k)## in every
single loop everywhere in your program and only use libraries which also do
this and never forget to do it ever; or
- don't write to Object.prototype.
Of these, the first option is terrible garbage. Go with the second option.
= Sparse Arrays =
Another wrench in this mess is that Arrays aren't precisely like lists, because
they do have indexes and may be sparse:
var a = [];
a[2] = 1;
console.log(a); // [undefined, undefined, 1]
The correct way to deal with this is:
for (var ii = 0; ii < list.length; ii++) {
if (list[ii] == undefined) {
continue;
}
f(list[ii]);
}
Avoid sparse arrays if possible.
= Ordered Maps =
If you need an ordered map, you need to have a map for key-value associations
and a list for key order. Don't try to build an ordered map using one Object or
one Array. This generally applies for other complicated datatypes, as well; you
need to build them out of more than one primitive.
diff --git a/src/docs/user/flavortext/javascript_pitfalls.diviner b/src/docs/flavor/javascript_pitfalls.diviner
similarity index 99%
rename from src/docs/user/flavortext/javascript_pitfalls.diviner
rename to src/docs/flavor/javascript_pitfalls.diviner
index a4815e8f3a..fb9af3c197 100644
--- a/src/docs/user/flavortext/javascript_pitfalls.diviner
+++ b/src/docs/flavor/javascript_pitfalls.diviner
@@ -1,87 +1,87 @@
@title Javascript Pitfalls
-@group flavortext
+@group javascript
This document discusses pitfalls and flaws in the Javascript language, and how
to avoid, work around, or at least understand them.
= Implicit Semicolons =
Javascript tries to insert semicolons if you forgot them. This is a pretty
horrible idea. Notably, it can mask syntax errors by transforming subexpressions
on their own lines into statements with no effect:
lang=js
string = "Here is a fairly long string that does not fit on one "
"line. Note that I forgot the string concatenation operators "
"so this will compile and execute with the wrong behavior. ";
Here's what ECMA262 says about this:
When, as the program is parsed ..., a token ... is encountered that is not
allowed by any production of the grammar, then a semicolon is automatically
inserted before the offending token if one or more of the following conditions
is true: ...
To protect yourself against this "feature", don't use it. Always explicitly
insert semicolons after each statement. You should also prefer to break lines in
places where insertion of a semicolon would not make the unparseable parseable,
usually after operators.
= ##with## is Bad News =
##with## is a pretty bad feature, for this reason among others:
with (object) {
property = 3; // Might be on object, might be on window: who knows.
}
Avoid ##with##.
= ##arguments## is not an Array =
You can convert ##arguments## to an array using JX.$A() or similar. Note that
you can pass ##arguments## to Function.prototype.apply() without converting it.
= Object, Array, and iteration are needlessly hard =
There is essentially only one reasonable, consistent way to use these primitives
but it is not obvious. Navigate these troubled waters with
@{article:Javascript Object and Array}.
= typeof null == "object" =
This statement is true in Javascript:
typeof null == 'object'
This is pretty much a bug in the language that can never be fixed now.
= Number, String, and Boolean objects =
Like Java, Javascript has primitive versions of number, string, and boolean,
and object versions. In Java, there's some argument for this distinction. In
Javascript, it's pretty much completely worthless and the behavior of these
objects is wrong. String and Boolean in particular are essentially unusable:
lang=js
"pancake" == "pancake"; // true
new String("pancake") == new String("pancake"); // false
var b = new Boolean(false);
b; // Shows 'false' in console.
!b; // ALSO shows 'false' in console.
!b == b; // So this is true!
!!b == !b // Negate both sides and it's false! FUCK!
if (b) {
// Better fucking believe this will get executed.
}
There is no advantage to using the object forms (the primitive forms behave like
objects and can have methods and properties, and inherit from Array.prototype,
Number.prototype, etc.) and their logical behavior is at best absurd and at
worst strictly wrong.
**Never use** ##new Number()##, ##new String()## or ##new Boolean()## unless
your Javascript is God Tier and you are absolutely sure you know what you are
doing.
diff --git a/src/docs/user/flavortext/php_pitfalls.diviner b/src/docs/flavor/php_pitfalls.diviner
similarity index 99%
rename from src/docs/user/flavortext/php_pitfalls.diviner
rename to src/docs/flavor/php_pitfalls.diviner
index 81750a3afa..c51b459910 100644
--- a/src/docs/user/flavortext/php_pitfalls.diviner
+++ b/src/docs/flavor/php_pitfalls.diviner
@@ -1,301 +1,301 @@
@title PHP Pitfalls
-@group flavortext
+@group php
This document discusses difficult traps and pitfalls in PHP, and how to avoid,
work around, or at least understand them.
= array_merge() in Incredibly Slow When Merging A List of Arrays =
If you merge a list of arrays like this:
COUNTEREXAMPLE
$result = array();
foreach ($list_of_lists as $one_list) {
$result = array_merge($result, $one_list);
}
...your program now has a huge runtime because it generates a large number of
intermediate arrays and copies every element it has previously seen each time
you iterate.
In a libphutil environment, you can use @{function@libphutil:array_mergev}
instead.
= var_export() Hates Baby Animals =
If you try to var_export() an object that contains recursive references, your
program will terminate. You have no chance to intercept or react to this or
otherwise stop it from happening. Avoid var_export() unless you are certain
you have only simple data. You can use print_r() or var_dump() to display
complex variables safely.
= isset(), empty() and Truthiness =
A value is "truthy" if it evaluates to true in an ##if## clause:
$value = something();
if ($value) {
// Value is truthy.
}
If a value is not truthy, it is "falsey". These values are falsey in PHP:
null // null
0 // integer
0.0 // float
"0" // string
"" // empty string
false // boolean
array() // empty array
Disregarding some bizarre edge cases, all other values are truthy. Note that
because "0" is falsey, this sort of thing (intended to prevent users from making
empty comments) is wrong in PHP:
COUNTEREXAMPLE
if ($comment_text) {
make_comment($comment_text);
}
This is wrong because it prevents users from making the comment "0". //THIS
COMMENT IS TOTALLY AWESOME AND I MAKE IT ALL THE TIME SO YOU HAD BETTER NOT
BREAK IT!!!// A better test is probably strlen().
In addition to truth tests with ##if##, PHP has two special truthiness operators
which look like functions but aren't: empty() and isset(). These operators help
deal with undeclared variables.
In PHP, there are two major cases where you get undeclared variables -- either
you directly use a variable without declaring it:
COUNTEREXAMPLE
function f() {
if ($not_declared) {
// ...
}
}
...or you index into an array with an index which may not exist:
COUNTEREXAMPLE
function f(array $mystery) {
if ($mystery['stuff']) {
// ...
}
}
When you do either of these, PHP issues a warning. Avoid these warnings by using
empty() and isset() to do tests that are safe to apply to undeclared variables.
empty() evaluates truthiness exactly opposite of if(). isset() returns true for
everything except null. This is the truth table:
VALUE if() empty() isset()
null false true false
0 false true true
0.0 false true true
"0" false true true
"" false true true
false false true true
array() false true true
EVERYTHING ELSE true false true
The value of these operators is that they accept undeclared variables and do not
issue a warning. Specifically, if you try to do this you get a warning:
COUNTEREXAMPLE
if ($not_previously_declared) { // PHP Notice: Undefined variable!
// ...
}
But these are fine:
if (empty($not_previously_declared)) { // No notice, returns true.
// ...
}
if (isset($not_previously_declared)) { // No notice, returns false.
// ...
}
So, isset() really means is_declared_and_is_set_to_something_other_than_null().
empty() really means is_falsey_or_is_not_declared(). Thus:
- If a variable is known to exist, test falsiness with if (!$v), not empty().
In particular, test for empty arrays with if (!$array). There is no reason
to ever use empty() on a declared variable.
- When you use isset() on an array key, like isset($array['key']), it will
evaluate to "false" if the key exists but has the value null! Test for index
existence with array_key_exists().
Put another way, use isset() if you want to type "if ($value !== null)" but are
testing something that may not be declared. Use empty() if you want to type
"if (!$value)" but you are testing something that may not be declared.
= usort(), uksort(), and uasort() are Slow =
This family of functions is often extremely slow for large datasets. You should
avoid them if at all possible. Instead, build an array which contains surrogate
keys that are naturally sortable with a function that uses native comparison
(e.g., sort(), asort(), ksort(), or natcasesort()). Sort this array instead, and
use it to reorder the original array.
In a libphutil environment, you can often do this easily with
@{function@libphutil:isort} or @{function@libphutil:msort}.
= array_intersect() and array_diff() are Also Slow =
These functions are much slower for even moderately large inputs than
array_intersect_key() and array_diff_key(), because they can not make the
assumption that their inputs are unique scalars as the ##key## varieties can.
Strongly prefer the ##key## varieties.
= array_uintersect() and array_udiff() are Definitely Slow Too =
These functions have the problems of both the ##usort()## family and the
##array_diff()## family. Avoid them.
= foreach() Does Not Create Scope =
Variables survive outside of the scope of foreach(). More problematically,
references survive outside of the scope of foreach(). This code mutates
##$array## because the reference leaks from the first loop to the second:
COUNTEREXAMPLE
$array = range(1, 3);
echo implode(',', $array); // Outputs '1,2,3'
foreach ($array as &$value) {}
echo implode(',', $array); // Outputs '1,2,3'
foreach ($array as $value) {}
echo implode(',', $array); // Outputs '1,2,2'
The easiest way to avoid this is to avoid using foreach-by-reference. If you do
use it, unset the reference after the loop:
foreach ($array as &$value) {
// ...
}
unset($value);
= unserialize() is Incredibly Slow on Large Datasets =
The performance of unserialize() is nonlinear in the number of zvals you
unserialize, roughly O(N^2).
zvals approximate time
10000 5ms
100000 85ms
1000000 8,000ms
10000000 72 billion years
= call_user_func() Breaks References =
If you use call_use_func() to invoke a function which takes parameters by
reference, the variables you pass in will have their references broken and will
emerge unmodified. That is, if you have a function that takes references:
function add_one(&$v) {
$v++;
}
...and you call it with call_user_func():
COUNTEREXAMPLE
$x = 41;
call_user_func('add_one', $x);
...##$x## will not be modified. The solution is to use call_user_func_array()
and wrap the reference in an array:
$x = 41;
call_user_func_array(
'add_one',
array(&$x)); // Note '&$x'!
This will work as expected.
= You Can't Throw From __toString() =
If you throw from __toString(), your program will terminate uselessly and you
won't get the exception.
= An Object Can Have Any Scalar as a Property =
Object properties are not limited to legal variable names:
$property = '!@#$%^&*()';
$obj->$property = 'zebra';
echo $obj->$property; // Outputs 'zebra'.
So, don't make assumptions about property names.
= There is an (object) Cast =
You can cast a dictionary into an object.
$obj = (object)array('flavor' => 'coconut');
echo $obj->flavor; // Outputs 'coconut'.
echo get_class($obj); // Outputs 'stdClass'.
This is occasionally useful, mostly to force an object to become a Javascript
dictionary (vs a list) when passed to json_encode().
= Invoking "new" With an Argument Vector is Really Hard =
If you have some ##$class_name## and some ##$argv## of constructor
arguments and you want to do this:
new $class_name($argv[0], $argv[1], ...);
...you'll probably invent a very interesting, very novel solution that is very
wrong. In a libphutil environment, solve this problem with
@{function@libphutil:newv}. Elsewhere, copy newv()'s implementation.
= Equality is not Transitive =
This isn't terribly surprising since equality isn't transitive in a lot of
languages, but the == operator is not transitive:
$a = ''; $b = 0; $c = '0a';
$a == $b; // true
$b == $c; // true
$c == $a; // false!
When either operand is an integer, the other operand is cast to an integer
before comparison. Avoid this and similar pitfalls by using the === operator,
which is transitive.
= All 676 Letters in the Alphabet =
This doesn't do what you'd expect it to do in C:
for ($c = 'a'; $c <= 'z'; $c++) {
// ...
}
This is because the successor to 'z' is 'aa', which is "less than" 'z'. The
loop will run for ~700 iterations until it reaches 'zz' and terminates. That is,
##$c## will take on these values:
a
b
...
y
z
aa // loop continues because 'aa' <= 'z'
ab
...
mf
mg
...
zw
zx
zy
zz // loop now terminates because 'zz' > 'z'
Instead, use this loop:
foreach (range('a', 'z') as $c) {
// ...
}
diff --git a/src/docs/user/flavortext/please_please_please.diviner b/src/docs/flavor/please_please_please.diviner
similarity index 98%
rename from src/docs/user/flavortext/please_please_please.diviner
rename to src/docs/flavor/please_please_please.diviner
index 35c83cfa1e..2391a55ba3 100644
--- a/src/docs/user/flavortext/please_please_please.diviner
+++ b/src/docs/flavor/please_please_please.diviner
@@ -1,78 +1,78 @@
@title Please Please Please
-@group flavortext
+@group sundry
Please read this document.
When you send a message that says "I got an error when I tried to do something":
= Please Please Please =
Please copy and paste the text of the error.
Please please please. It is very helpful. So please please please please please
please do this.
I want to help. I really do. But it is very difficult when you don't include the
error text and only allude to how traumatizing reading that text was for you. It
is basically inviting me to troll you.
I am only human. I have only so much restraint.
So please include the error message in your message, before you send it.
Or even after you send it, if you realize you forgot. That would be okay too. I
would understand.
Just please do this.
Please.
= Thank You =
Thank you for reading this note.
= Appendix: A Collection of Messages Wherein The Text of the Error is Not Present =
Please
COUNTEREXAMPLE
Subject: help
X-Priority: 1
it dun work
please
COUNTEREXAMPLE
Subject: Error
I got an error when I ran it. Why?
please
COUNTEREXAMPLE
Subject: error when running program
I ran the program and it gave me an error.
What do I do now?
please
COUNTEREXAMPLE
Subject: so many errors
I got an error and it had instructions in it. I tried to follow the
instructions but I got a different error. Are you sure this works?
please
COUNTEREXAMPLE
Subject: confusing error
i got an error but it was confusing
can you explain what it means?
please
COUNTEREXAMPLE
Subject: Irony
There is a spelling error in the error message I received.
please include the text of the error message.
diff --git a/src/docs/user/flavortext/project_history.diviner b/src/docs/flavor/project_history.diviner
similarity index 99%
rename from src/docs/user/flavortext/project_history.diviner
rename to src/docs/flavor/project_history.diviner
index ce4916dbc0..bfdbe2682e 100644
--- a/src/docs/user/flavortext/project_history.diviner
+++ b/src/docs/flavor/project_history.diviner
@@ -1,60 +1,60 @@
@title Phabricator Project History
-@group flavortext
+@group lore
A riveting tale of adventure. In this document, I refer to worldly and
sophisticated engineer Evan Priestley as "I", which is only natural as I am he.
This document is mostly just paragraph after paragraph of self-aggrandizement.
= In The Beginning =
I wrote the original version of Differential in one night at a Facebook
Hackathon in April or May 2007, along with Luke Shepard. I joined the company in
April and code review was already an established and mostly-mandatory part of
the culture, but it happened over email and was inefficient and hard to keep
track of. I remember feeling like I was spending a lot of time waiting for code
review to happen, which was a major motivator for building the tool.
The original name of the tool was "Diffcamp". Some time earlier there had been
an attempt to create a project management tool that was a sort of hybrid between
Trac and Basecamp called "Traccamp". Since we were writing the code review tool
at the height of the brief popularity Traccamp enjoyed, we integrated and called
the new tool Diffcamp even though it had no relation to Basecamp. Traccamp fell
by the wayside shortly thereafter and was eventually removed.
However, Diffcamp didn't share its fate. We spent some more time working on it
and got good enough to win hearts and minds over emailing diffs around and was
soon the de facto method of code review at Facebook.
= The Long Bloat =
For the next two and a half years, Diffcamp grew mostly organically and gained a
number of features like inline commenting, CLI support and git support (Facebook
was 100% SVN in early 2007 but 90%+ of Engineers worked primarily in git with
SVN bridging by 2010). As these patches were contributed pretty much randomly,
it also gained a lot of performance problems, usability issues, and bugs.
Through 2007 and 2008 I worked mostly on frontend and support infrastructure;
among other things, I wrote a static resource management system called Haste. In
2009 I worked on the Facebook Lite site, where I built the Javelin Javascript
library and an MVC-flavored framework called Alite.
But by early 2010, Diffcamp was in pretty bad shape. Two years of having random
features grafted onto it without real direction had left it slow and difficult
to use. Internal feedback on the tool was pretty negative, with a lot of
complaints about performance and stability. The internal XTools team had made
inroads at fixing these problems in late 2009, but they were stretched thin and
the tool had become a sprawling landscape of architectural and implementation
problems.
= Differential =
I joined the new Dev Tools team around February 2010 and took over Diffcamp. I
renamed it to Differential, moved it to a new Alite-based infrastructure with
Javelin, and started making it somewhat less terrible. I eventually wrote
Diffusion and build Herald to replace a very difficult-to-use predecessor. These
tools were less negatively received than the older versions. By December 2010 I
started open sourcing them; Haste became //Celerity// and Alite became
//Aphront//. I wrote Maniphest to track open issues with the project in January
or February and we open sourced Phabricator in late April, shortly after I left
Facebook.
diff --git a/src/docs/user/flavortext/recommendations_on_branching.diviner b/src/docs/flavor/recommendations_on_branching.diviner
similarity index 99%
rename from src/docs/user/flavortext/recommendations_on_branching.diviner
rename to src/docs/flavor/recommendations_on_branching.diviner
index ea5088a786..38d196ad9b 100644
--- a/src/docs/user/flavortext/recommendations_on_branching.diviner
+++ b/src/docs/flavor/recommendations_on_branching.diviner
@@ -1,188 +1,188 @@
@title Recommendations on Branching
-@group flavortext
+@group review
Project recommendations on how to organize branches.
This document discusses organizing branches in your remote/origin for feature
development and release management, not the use of local branches in Git or
queues or bookmarks in Mercurial.
This document is purely advisory. Phabricator works with a variety of branching
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 branching strategy used by Facebook and Phabricator to
develop software. It scales well and removes the pain associated with most
branching strategies. This strategy is most applicable to web applications, and
may be less applicable to other types of software. The basics are:
- Never put feature branches in the remote/origin/trunk.
- Control access to new features with runtime configuration, not branching.
The next sections describe these points in more detail, explaining why you
should consider abandoning feature branches and how to build runtime access
controls for features.
= Feature Branches =
Suppose you are developing a new feature, like a way for users to "poke" each
other. A traditional strategy is to create a branch for this feature in the
remote (say, "poke_branch"), develop the feature on the branch over some period
of time (say, a week or a month), and then merge the entire branch back into
master/default/trunk when the feature is complete.
This strategy has some drawbacks:
- You have to merge. Merging can be painful and error prone, especially if the
feature takes a long time to develop. Reducing merge pain means spending
time merging master into the branch regularly. As branches fall further
out of sync, merge pain/risk tends to increase.
- This strategy generally aggregates risk into a single high-risk merge event
at the end of development. It does this both explicitly (all the code lands
at once) and more subtly: since commits on the branch aren't going live any
time soon, it's easy to hold them to a lower bar of quality.
- When you have multiple feature branches, it's impossible to test
interactions between the features until they are merged.
- You generally can't A/B test code in feature branches, can't roll it out to
a small percentage of users, and can't easily turn it on for just employees
since it's in a separate branch.
Of course, it also has some advantages:
- If the new feature replaces an older feature, the changes can delete the
older feature outright, or at least transition from the old feature to the
new feature fairly rapidly.
- The chance that this code will impact production before the merge is nearly
zero (it normally requires substantial human error). This is the major
reason to do it at all.
Instead, consider abandoning all use of feature branching. The advantages are
straightforward:
- You don't have to do merges.
- Risk is generally spread out more evenly into a large number of very small
risks created as each commit lands.
- You can test interactions between features in development easily.
- You can A/B test and do controlled rollouts easily.
But it has some tradeoffs:
- If a new feature replaces an older feature, both have to exist in the same
codebase for a while. But even with feature branching, you generally have to
do almost all this work anyway to avoid situations where you flip a switch
and can't undo it.
- You need an effective way to control access to features so they don't launch
before they're ready. Even with this, there is a small risk a feature may
launch or leak because of a smaller human error than would be required with
feature branching. However, for most applications, this isn't a big deal.
This second point is a must-have, but entirely tractable. The next section
describes how to build it, so you can stop doing feature branching and never
deal with the pain and risk of merging again.
= Controlling Access to Features =
Controlling access to features is straightforward: build some kind of runtime
configuration which defines which features are visible, based on the tier
(e.g., development, testing or production?) code is deployed on, the logged in
user, global configuration, random buckets, A/B test groups, or whatever else.
Your code should end up looking something like this:
if (is_feature_launched('poke')) {
show_poke();
}
Behind that is some code which knows about the 'poke' feature and can go lookup
configuration to determine if it should be visible or not. Facebook has a very
sophisticated system for this (called GateKeeper) which also integrates with A/B
tests, allows you to define complicated business rules, etc.
You don't need this in the beginning. Before GateKeeper, Facebook used a much
simpler system (called Sitevars) to handle this. Here are some resources
describing similar systems:
- There's a high-level overview of Facebook's system in this 2011 tech talk:
[[http://techcrunch.com/2011/05/30/facebook-source-code/ | Facebook Push
Tech Talk]].
- Flickr described their similar system in a 2009 blog post here:
[[http://code.flickr.com/blog/2009/12/02/flipping-out/ | Flickr Feature
Flags and Feature Flippers]].
- Disqus described their similar system in a 2010 blog post here:
[[http://blog.disqus.com/post/789540337/partial-deployment-with-feature-switches |
Disqus Feature Switches]].
- Forrst describes their similar system in a 2010 blog post here:
[[http://blog.forrst.com/post/782356699/how-we-deploy-new-features-on-forrst |
Forrst Buckets]].
- Martin Fowler discusses these systems in a 2010 blog post here:
[[http://martinfowler.com/bliki/FeatureToggle.html |
Martin Fowler's FeatureToggle]].
- Phabricator just adds config options but defaults them to off. When
developing, we turn them on locally. Once a feature is ready, we default it
on. We have a vastly less context to deal with than most projects, however,
and sometimes get away with simply not linking new features in the UI until
they mature (it's open source anyway so there's no value in hiding things).
When building this system there are a few things to avoid, mostly related to not
letting the complexity of this system grow too wildly:
- Facebook initially made it very easy to turn things on to everyone by
accident in GateKeeper. Don't do this. The UI should make it obvious when
you're turning something on or off, and default to off.
- Since GateKeeper is essentially a runtime business rules engine, it was
heavily abused to effectively execute code living in a database. Avoid this
through simpler design or a policy of sanity.
- Facebook allowed GateKeeper rules to depend on other GateKeeper rules
(for example, 'new_profile_tour' is launched if 'new_profile' is launched)
but did not perform cycle detection, and then sat on a bug describing
how to introduce a cycle and bring the site down for a very long time,
until someone introduced a cycle and brought the site down. If you implement
dependencies, implement cycle detection.
- Facebook implemented some very expensive GateKeeper conditions and was
spending 100+ ms per page running complex rulesets to do launch checks for a
number of months in 2009. Keep an eye on how expensive your checks are.
That said, not all complexity is bad:
- Allowing features to have states like "3%" instead of just "on" or "off"
allows you to roll out features gradually and watch for trouble (e.g.,
services collapsing from load).
- Building a control panel where you hit "Save" and all production servers
immediately reflect the change allows you to quickly turn things off if
there are problems.
- If you perform A/B testing, integrating A/B tests with feature rollouts
is probably a natural fit.
- Allowing new features to be launched to all employees before they're
launched to the world is a great way to get feedback and keep everyone
in the loop.
Adopting runtime feature controls increases the risk of features leaking (or
even launching) before they're ready. This is generally a small risk which is
probably reasonable for most projects to accept, although it might be
unacceptable for some projects. There are some ways you can mitigate this
risk:
- Essentially every launch/leak at Facebook was because someone turned on
a feature by accident when they didn't mean to. The control panel made this
very easy: features defaulted to "on", and if you tried to do something
common like remove yourself from the test group for a feature you could
easily launch it to the whole world. Design the UI defensively so that it
is hard to turn features on to everyone and/or obvious when a feature is
launching and this shouldn't be a problem.
- The rest were through CSS or JS changes that mentioned new features being
shipped to the client as part of static resource packaging or because
the code was just added to existing files. If this is a risk you're
concerned about, consider integration with static resource management.
In general, you can start with a very simple system and expand it as it makes
sense. Even a simple system can let you move away from feature branches.
= Next Steps =
Continue by:
- reading recommendations on structuring revision control with
@{article:Recommendations on Revision Control}; or
- reading recommendations on structuring changes with
@{article:Writing Reviewable Code}.
diff --git a/src/docs/user/flavortext/recommendations_on_revision_control.diviner b/src/docs/flavor/recommendations_on_revision_control.diviner
similarity index 99%
rename from src/docs/user/flavortext/recommendations_on_revision_control.diviner
rename to src/docs/flavor/recommendations_on_revision_control.diviner
index 66b7bfc9d9..0cb0eb5e6c 100644
--- a/src/docs/user/flavortext/recommendations_on_revision_control.diviner
+++ b/src/docs/flavor/recommendations_on_revision_control.diviner
@@ -1,92 +1,92 @@
@title Recommendations on Revision Control
-@group flavortext
+@group review
Project recommendations on how to organize revision control.
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.
This is my (epriestley's) personal take on the issue and not necessarily
representative of the views of the Phabricator team as a whole.
= Overview =
There are a few ways to use SVN, a few ways to use Mercurial, and many many many
ways to use Git. Particularly with Git, every project does things differently,
and all these approaches are valid for small projects. When projects scale,
strategies which enforce **one idea is one commit** are better.
= One Idea is One Commit =
Choose a strategy where **one idea is one commit** in the authoritative
master/remote version of the repository. Specifically, this means that an entire
conceptual changeset ("add a foo widget") is represented in the remote as
exactly one commit (in some form), not a sequence of checkpoint commits.
- In SVN, this means don't ##commit## until after an idea has been completely
written. All reasonable SVN workflows naturally enforce this.
- In Git, this means squashing checkpoint commits as you go (with ##git commit
--amend##) or before pushing (with ##git rebase -i## or ##git merge
--squash##), or having a strict policy where your master/trunk contains only
merge commits and each is a merge between the old master and a branch which
represents a single idea. Although this preserves the checkpoint commits
along the branches, you can view master alone as a series of single-idea
commits.
- In Mercurial, you can use the "queues" extension before 2.2 or `--amend`
after Mercurial 2.2, or wait to commit until a change is complete (like
SVN), although the latter is not recommended. Without extensions, older
versions of Mercurial do not support liberal mutability doctrines (so you
can't ever combine checkpoint commits) and do not let you build a default
out of only merge commits, so it is not possible to have an authoritative
repository where one commit represents one idea in any real sense.
= Why This Matters =
A strategy where **one idea is one commit** has no real advantage over any other
strategy until your repository hits a velocity where it becomes critical. In
particular:
- Essentially all operations against the master/remote repository are about
ideas, not commits. When one idea is many commits, everything you do is more
complicated because you need to figure out which commits represent an idea
("the foo widget is broken, what do I need to revert?") or what idea is
ultimately represented by a commit ("commit af3291029 makes no sense, what
goal is this change trying to accomplish?").
- Release engineering is greatly simplified. Release engineers can pick or
drop ideas easily when each idea corresponds to one commit. When an idea
is several commits, it becomes easier to accidentally pick or drop half of
an idea and end up in a state which is virtually guaranteed to be wrong.
- Automated testing is greatly simplified. If each idea is one commit, you
can run automated tests against every commit and test failures indicate a
serious problem. If each idea is many commits, most of those commits
represent a known broken state of the codebase (e.g., a checkpoint with a
syntax error which was fixed in the next checkpoint, or with a
half-implemented idea).
- Understanding changes is greatly simplified. You can bisect to a break and
identify the entire idea trivially, without fishing forward and backward in
the log to identify the extents of the idea. And you can be confident in
what you need to revert to remove the entire idea.
- There is no clear value in having checkpoint commits (some of which are
guaranteed to be known broken versions of the repository) persist into the
remote. Consider a theoretical VCS which automatically creates a checkpoint
commit for every keystroke. This VCS would obviously be unusable. But many
checkpoint commits aren't much different, and conceptually represent some
relatively arbitrary point in the sequence of keystrokes that went into
writing a larger idea. Get rid of them or create an abstraction layer (merge
commits) which allows you to ignore them when you are trying to understand
the repository in terms of ideas (which is almost always).
All of these become problems only at scale. Facebook pushes dozens of ideas
every day and thousands on a weekly basis, and could not do this (at least, not
without more people or more errors) without choosing a repository strategy where
**one idea is one commit**.
= Next Steps =
Continue by:
- reading recommendations on structuring branches with
@{article:Recommendations on Branching}; or
- reading recommendations on structuring changes with
@{article:Writing Reviewable Code}.
diff --git a/src/docs/user/flavortext/soon_static_resources.diviner b/src/docs/flavor/soon_static_resources.diviner
similarity index 99%
rename from src/docs/user/flavortext/soon_static_resources.diviner
rename to src/docs/flavor/soon_static_resources.diviner
index 1a663fea26..78820c5081 100644
--- a/src/docs/user/flavortext/soon_static_resources.diviner
+++ b/src/docs/flavor/soon_static_resources.diviner
@@ -1,126 +1,126 @@
@title Things You Should Do Soon: Static Resources
-@group flavortext
+@group sundry
Over time, you'll write more JS and CSS and eventually need to put systems in
place to manage it.
This is part of @{article:Things You Should Do Soon}, which describes
architectural problems in web applications which you should begin to consider
before you encounter them.
= Manage Dependencies Automatically =
The naive way to add static resources to a page is to include them at the top
of the page, before rendering begins, by enumerating filenames. Facebook used to
work like that:
COUNTEREXAMPLE
<?php
require_js('js/base.js');
require_js('js/utils.js');
require_js('js/ajax.js');
require_js('js/dialog.js');
// ...
This was okay for a while but had become unmanageable by 2007. Because
dependencies were managed completely manually and you had to explicitly list
every file you needed in the right order, everyone copy-pasted a giant block
of this stuff into every page. The major problem this created was that each page
pulled in way too much JS, which slowed down frontend performance.
We moved to a system (called //Haste//) which declared JS dependencies in the
files using a docblock-like header:
/**
* @provides dialog
* @requires utils ajax base
*/
We annotated files manually, although theoretically you could use static
analysis instead (we couldn't realistically do that, our JS was pretty
unstructured). This allowed us to pull in the entire dependency chain of
component with one call:
require_static('dialog');
...instead of copy-pasting every dependency.
= Include When Used =
The other part of this problem was that all the resources were required at the
top of the page instead of when they were actually used. This meant two things:
- you needed to include every resource that //could ever// appear on a page;
- if you were adding something new to 2+ pages, you had a strong incentive to
put it in base.js.
So every page pulled in a bunch of silly stuff like the CAPTCHA code (because
there was one obscure workflow involving unverified users which could
theoretically show any user a CAPTCHA on any page) and every random thing anyone
had stuck in base.js.
We moved to a system where JS and CSS tags were output **after** page rendering
had run instead (they still appeared at the top of the page, they were just
prepended rather than appended before being output to the browser -- there are
some complexities here, but they are beyond the immediate scope), so
require_static() could appear anywhere in the code. Then we moved all the
require_static() calls to be proximate to their use sites (so dialog rendering
code would pull in dialog-related CSS and JS, for example, not any page which
might need a dialog), and split base.js into a bunch of smaller files.
= Packaging =
The biggest frontend performance killer in most cases is the raw number of HTTP
requests, and the biggest hammer for addressing it is to package related JS
and CSS into larger files, so you send down all the core JS code in one big file
instead of a lot of smaller ones. Once the other groundwork is in place, this is
a relatively easy change. We started with manual package definitions and
eventually moved to automatic generation based on production data.
= Caches and Serving Content =
In the simplest implementation of static resources, you write out a raw JS tag
with something like ##src="/js/base.js"##. This will break disastrously as you
scale, because clients will be running with stale versions of resources. There
are bunch of subtle problems (especially once you have a CDN), but the big one
is that if a user is browsing your site as you push/deploy, their client will
not make requests for the resources they already have in cache, so even if your
servers respond correctly to If-None-Match (ETags) and If-Modified-Since
(Expires) the site will appear completely broken to everyone who was using it
when you push a breaking change to static resources.
The best way to solve this problem is to version your resources in the URI,
so each version of a resource has a unique URI:
rsrc/af04d14/js/base.js
When you push, users will receive pages which reference the new URI so their
browsers will retrieve it.
**But**, there's a big problem, once you have a bunch of web frontends:
While you're pushing, a user may make a request which is handled by a server
running the new version of the code, which delivers a page with a new resource
URI. Their browser then makes a request for the new resource, but that request
is routed to a server which has not been pushed yet, which delivers an old
version of the resource. They now have a poisoned cache: old resource data for
a new resource URI.
You can do a lot of clever things to solve this, but the solution we chose at
Facebook was to serve resources out of a database instead of off disk. Before a
push begins, new resources are written to the database so that every server is
able to satisfy both old and new resource requests.
This also made it relatively easy to do processing steps (like stripping
comments and whitespace) in one place, and just insert a minified/processed
version of CSS and JS into the database.
= Reference Implementation: Celerity =
Some of the ideas discussed here are implemented in Phabricator's //Celerity//
system, which is essentially a simplified version of the //Haste// system used
by Facebook.
diff --git a/src/docs/user/flavortext/things_you_should_do_now.diviner b/src/docs/flavor/things_you_should_do_now.diviner
similarity index 99%
rename from src/docs/user/flavortext/things_you_should_do_now.diviner
rename to src/docs/flavor/things_you_should_do_now.diviner
index d446c741c2..b4681bd0ca 100644
--- a/src/docs/user/flavortext/things_you_should_do_now.diviner
+++ b/src/docs/flavor/things_you_should_do_now.diviner
@@ -1,138 +1,138 @@
@title Things You Should Do Now
-@group flavortext
+@group sundry
Describes things you should do now when building software, because the cost to
do them increases over time and eventually becomes prohibitive or impossible.
= Overview =
If you're building a hot new web startup, there are a lot of decisions to make
about what to focus on. Most things you'll build will take about the same amount
of time to build regardless of what order you build them in, but there are a few
technical things which become vastly more expensive to fix later.
If you don't do these things early in development, they'll become very hard or
impossible to do later. This is basically a list of things that would have saved
Facebook huge amounts of time and effort down the road if someone had spent
a tiny amount of time on them earlier in the development process.
See also @{article:Things You Should Do Soon} for things that scale less
drastically over time.
= Start IDs At a Gigantic Number =
If you're using integer IDs to identify data or objects, **don't** start your
IDs at 1. Start them at a huge number (e.g., 2^33) so that no object ID will
ever appear in any other role in your application (like a count, a natural
index, a byte size, a timestamp, etc). This takes about 5 seconds if you do it
before you launch and rules out a huge class of nasty bugs for all time. It
becomes incredibly difficult as soon as you have production data.
The kind of bug that this causes is accidental use of some other value as an ID:
COUNTEREXAMPLE
// Load the user's friends, returns a map of friend_id => true
$friend_ids = user_get_friends($user_id);
// Get the first 8 friends.
$first_few_friends = array_slice($friend_ids, 0, 8);
// Render those friends.
render_user_friends($user_id, array_keys($first_few_friends));
Because array_slice() in PHP discards array indices and renumbers them, this
doesn't render the user's first 8 friends but the users with IDs 0 through 7,
e.g. Mark Zuckerberg (ID 4) and Dustin Moskovitz (ID 6). If you have IDs in this
range, sooner or later something that isn't an ID will get treated like an ID
and the operation will be valid and cause unexpected behavior. This is
completely avoidable if you start your IDs at a gigantic number.
= Only Store Valid UTF-8 =
For the most part, you can ignore UTF-8 and unicode until later. However, there
is one aspect of unicode you should address now: store only valid UTF-8 strings.
Assuming you're storing data internally as UTF-8 (this is almost certainly the
right choice and definitely the right choice if you have no idea how unicode
works), you just need to sanitize all the data coming into your application and
make sure it's valid UTF-8.
If your application emits invalid UTF-8, other systems (like browsers) will
break in unexpected and interesting ways. You will eventually be forced to
ensure you emit only valid UTF-8 to avoid these problems. If you haven't
sanitized your data, you'll basically have two options:
- do a huge migration on literally all of your data to sanitize it; or
- forever sanitize all data on its way out on the read pathways.
As of 2011 Facebook is in the second group, and spends several milliseconds of
CPU time sanitizing every display string on its way to the browser, which
multiplies out to hundreds of servers worth of CPUs sitting in a datacenter
paying the price for the invalid UTF-8 in the databases.
You can likely learn enough about unicode to be confident in an implementation
which addresses this problem within a few hours. You don't need to learn
everything, just the basics. Your language probably already has a function which
does the sanitizing for you.
= Never Design a Blacklist-Based Security System =
When you have an alternative, don't design security systems which are default
permit, blacklist-based, or otherwise attempt to enumerate badness. When
Facebook launched Platform, it launched with a blacklist-based CSS filter, which
basically tried to enumerate all the "bad" parts of CSS and filter them out.
This was a poor design choice and lead to basically infinite security holes for
all time.
It is very difficult to enumerate badness in a complex system and badness is
often a moving target. Instead of trying to do this, design whitelist-based
security systems where you list allowed things and reject anything you don't
understand. Assume things are bad until you verify that they're OK.
It's tempting to design blacklist-based systems because they're easier to write
and accept more inputs. In the case of the CSS filter, the product goal was for
users to just be able to use CSS normally and feel like this system was no
different from systems they were familiar with. A whitelist-based system would
reject some valid, safe inputs and create product friction.
But this is a much better world than the alternative, where the blacklist-based
system fails to reject some dangerous inputs and creates //security holes//. It
//also// creates product friction because when you fix those holes you break
existing uses, and that backward-compatibility friction makes it very difficult
to move the system from a blacklist to a whitelist. So you're basically in
trouble no matter what you do, and have a bunch of security holes you need to
unbreak immediately, so you won't even have time to feel sorry for yourself.
Designing blacklist-based security is one of the worst now-vs-future tradeoffs
you can make. See also "The Six Dumbest Ideas in Computer Security":
http://www.ranum.com/security/computer_security/
= Fail Very Loudly when SQL Syntax Errors Occur in Production =
This doesn't apply if you aren't using SQL, but if you are: detect when a query
fails because of a syntax error (in MySQL, it is error 1064). If the failure
happened in production, fail in the loudest way possible. (I implemented this in
2008 at Facebook and had it just email me and a few other people directly. The
system was eventually refined.)
This basically creates a high-signal stream that tells you where you have SQL
injection holes in your application. It will have some false positives and could
theoretically have false negatives, but at Facebook it was pretty high signal
considering how important the signal is.
Of course, the real solution here is to not have SQL injection holes in your
application, ever. As far as I'm aware, this system correctly detected the one
SQL injection hole we had from mid-2008 until I left in 2011, which was in a
hackathon project on an underisolated semi-production tier and didn't use the
query escaping system the rest of the application does.
Hopefully, whatever language you're writing in has good query libraries that
can handle escaping for you. If so, use them. If you're using PHP and don't have
a solution in place yet, the Phabricator implementation of qsprintf() is similar
to Facebook's system and was successful there.
diff --git a/src/docs/user/flavortext/things_you_should_do_soon.diviner b/src/docs/flavor/things_you_should_do_soon.diviner
similarity index 96%
rename from src/docs/user/flavortext/things_you_should_do_soon.diviner
rename to src/docs/flavor/things_you_should_do_soon.diviner
index 3e6a019229..e1d960fd5e 100644
--- a/src/docs/user/flavortext/things_you_should_do_soon.diviner
+++ b/src/docs/flavor/things_you_should_do_soon.diviner
@@ -1,17 +1,17 @@
@title Things You Should Do Soon
-@group flavortext
+@group sundry
Describes things you should start thinking about soon, because scaling will
be easier if you put a plan in place.
= Overview =
Stop! Don't do any of this yet. Go do @{article:Things You Should Do Now} first.
Then you can come back and read about these things. These are basically some
problems you'll probably eventually encounter when building a web application
that might be good to start thinking about now.
= Things You Should Do Soon =
- @{article:Things You Should Do Soon: Static Resources}
diff --git a/src/docs/user/flavortext/writing_reviewable_code.diviner b/src/docs/flavor/writing_reviewable_code.diviner
similarity index 99%
rename from src/docs/user/flavortext/writing_reviewable_code.diviner
rename to src/docs/flavor/writing_reviewable_code.diviner
index 687f7233a2..7e840b7b6d 100644
--- a/src/docs/user/flavortext/writing_reviewable_code.diviner
+++ b/src/docs/flavor/writing_reviewable_code.diviner
@@ -1,163 +1,163 @@
@title Writing Reviewable Code
-@group flavortext
+@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.
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}.

File Metadata

Mime Type
text/x-diff
Expires
Mon, Jul 28, 2:03 AM (1 w, 19 h ago)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
186340
Default Alt Text
(94 KB)

Event Timeline

styx copyright 2020 sirocyl / ellie (kymkdd) / ncommander / styx contributors
Use of other names and brands is not intended to reflect a claim of, or right to, use the name or brand.
styx is an operating system using the Linux kernel and components, and "styx Linux" as a phrase does not reflect the inclusion of the Linux brand or trademark in the styx project.
Linux® is the registered trademark of Linus Torvalds in the U.S. and other countries.
Terms and Conditions (TODO: fix this!)