Minor tidying of DivinerWorkflow classes

Summary: Minor tidying and modernizing a few things.

Test Plan: Ran `./bin/diviner atomize` and `./bin/diviner generate`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11587

src/applications/diviner/workflow/DivinerAtomizeWorkflow.php

@@ -11,7 +11,7 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
           array(
             'name' => 'atomizer',
             'param' => 'class',
-            'help' => pht('Specify a subclass of DivinerAtomizer.'),
+            'help' => pht('Specify a subclass of %s.', 'DivinerAtomizer'),
           ),
           array(
             'name' => 'book',
@@ -36,7 +36,8 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
 
     $atomizer_class = $args->getArg('atomizer');
     if (!$atomizer_class) {
-      throw new Exception('Specify an atomizer class with --atomizer.');
+      throw new Exception(
+        pht('Specify an atomizer class with %s.', '--atomizer'));
     }
 
     $symbols = id(new PhutilSymbolLoader())
@@ -46,15 +47,17 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
       ->selectAndLoadSymbols();
     if (!$symbols) {
       throw new Exception(
-        "Atomizer class '{$atomizer_class}' must be a concrete subclass of ".
+        pht(
-        "DivinerAtomizer.");
+          "Atomizer class '%s' must be a concrete subclass of %s.",
+          $atomizer_class,
+          'DivinerAtomizer'));
     }
 
     $atomizer = newv($atomizer_class, array());
 
     $files = $args->getArg('files');
     if (!$files) {
-      throw new Exception('Specify one or more files to atomize.');
+      throw new Exception(pht('Specify one or more files to atomize.'));
     }
 
     $file_atomizer = new DivinerFileAtomizer();
@@ -80,10 +83,10 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
       $data = Filesystem::readFile($abs_path);
 
       if (!$this->shouldAtomizeFile($file, $data)) {
-        $console->writeLog("Skipping %s...\n", $file);
+        $console->writeLog("%s\n", pht('Skipping %s...', $file));
         continue;
       } else {
-        $console->writeLog("Atomizing %s...\n", $file);
+        $console->writeLog("%s\n", pht('Atomizing %s...', $file));
       }
 
       $context['group'] = null;
@@ -98,7 +101,8 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
       $all_atoms[] = $file_atoms;
 
       if (count($file_atoms) !== 1) {
-        throw new Exception('Expected exactly one atom from file atomizer.');
+        throw new Exception(
+          pht('Expected exactly one atom from file atomizer.'));
       }
       $file_atom = head($file_atoms);
 
@@ -121,17 +125,15 @@ final class DivinerAtomizeWorkflow extends DivinerWorkflow {
     if ($args->getArg('ugly')) {
       $json = json_encode($all_atoms);
     } else {
-      $json_encoder = new PhutilJSON();
+      $json = id(new PhutilJSON())->encodeFormatted($all_atoms);
-      $json = $json_encoder->encodeFormatted($all_atoms);
     }
 
     $console->writeOut('%s', $json);
-
     return 0;
   }
 
   private function shouldAtomizeFile($file_name, $file_data) {
-    return (strpos($file_data, '@'.'undivinable') === false);
+    return strpos($file_data, '@'.'undivinable') === false;
   }
 
 }

src/applications/diviner/workflow/DivinerGenerateWorkflow.php

@@ -12,12 +12,12 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
         array(
           array(
             'name' => 'clean',
-            'help' => 'Clear the caches before generating documentation.',
+            'help' => pht('Clear the caches before generating documentation.'),
           ),
           array(
             'name' => 'book',
             'param' => 'path',
-            'help' => 'Path to a Diviner book configuration.',
+            'help' => pht('Path to a Diviner book configuration.'),
           ),
         ));
   }
@@ -52,9 +52,10 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
       if (!$books) {
         throw new PhutilArgumentUsageException(
           pht(
-            "There are no Diviner '.book' files anywhere beneath the ".
+            "There are no Diviner '%s' files anywhere beneath the current ".
-            "current directory. Use '--book <book>' to specify a ".
+            "directory. Use '%s' to specify a documentation book to generate.",
-            "documentation book to generate."));
+            '.book',
+            '--book <book>'));
       } else {
         $this->log(pht('Found %s book(s).', new PhutilNumber(count($books))));
       }
@@ -85,13 +86,13 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     // amount of work we can, so that regenerating documentation after minor
     // changes is quick.
     //
-    // ATOM CACHE
+    // = ATOM CACHE =
     //
     // In the first stage, we find all the direct changes to source code since
     // the last run. This stage relies on two data structures:
     //
-    //  - File Hash Map: map<file_hash, node_hash>
+    //  - File Hash Map: `map<file_hash, node_hash>`
-    //  - Atom Map: map<node_hash, true>
+    //  - Atom Map: `map<node_hash, true>`
     //
     // First, we hash all the source files in the project to detect any which
     // have changed since the previous run (i.e., their hash is not present in
@@ -111,18 +112,18 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     // its methods). The File Hash Map contains an exhaustive list of all atoms
     // with type "file", but not child atoms of those top-level atoms.)
     //
-    // GRAPH CACHE
+    // = GRAPH CACHE =
     //
     // We now know which atoms exist, and can compare the Atom Map to some
     // existing cache to figure out what has changed. However, this isn't
     // sufficient to figure out which documentation actually needs to be
-    // regnerated, because atoms depend on other atoms. For example, if "B
+    // regenerated, because atoms depend on other atoms. For example, if `B
-    // extends A" and the definition for A changes, we need to regenerate the
+    // extends A` and the definition for `A` changes, we need to regenerate the
-    // documentation in B. Similarly, if X links to Y and Y changes, we should
+    // documentation in `B`. Similarly, if `X` links to `Y` and `Y` changes, we
-    // regenerate X. (In both these cases, the documentation for the connected
+    // should regenerate `X`. (In both these cases, the documentation for the
-    // atom may not acutally change, but in some cases it will, and the extra
+    // connected atom may not actually change, but in some cases it will, and
-    // work we need to do is generally very small compared to the size of the
+    // the extra work we need to do is generally very small compared to the
-    // project.)
+    // size of the project.)
     //
     // To figure out which other nodes have changed, we compute a "graph hash"
     // for each node. This hash combines the "node hash" with the node hashes
@@ -134,26 +135,25 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     //
     // In this stage, we rely on three data structures:
     //
-    //  - Symbol Map: map<node_hash, symbol_hash>
+    //  - Symbol Map: `map<node_hash, symbol_hash>`
-    //  - Edge Map: map<node_hash, list<symbol_hash>>
+    //  - Edge Map: `map<node_hash, list<symbol_hash>>`
-    //  - Graph Map: map<node_hash, graph_hash>
+    //  - Graph Map: `map<node_hash, graph_hash>`
     //
     // Calculating the graph hash requires several steps, because we need to
     // figure out which nodes an atom is attached to. The atom contains symbolic
-    // references to other nodes by name (e.g., "extends SomeClass") in the form
+    // references to other nodes by name (e.g., `extends SomeClass`) in the form
-    // of DivinerAtomRefs. We can also build a symbolic reference for any atom
+    // of @{class:DivinerAtomRefs}. We can also build a symbolic reference for
-    // from the atom itself. Each DivinerAtomRef generates a symbol hash,
+    // any atom from the atom itself. Each @{class:DivinerAtomRef} generates a
-    // which ends with an "S", for "symbol".
+    // symbol hash, which ends with an "S", for "symbol".
     //
     // First, we update the symbol map. We remove (and mark dirty) any symbols
     // associated with node hashes which no longer exist (e.g., old/dead nodes).
     // Second, we add (and mark dirty) any symbols associated with new nodes.
     // We also add edges defined by new nodes to the graph.
     //
-    // We initialize a list of dirty nodes to the list of new nodes, then
+    // We initialize a list of dirty nodes to the list of new nodes, then find
-    // find all nodes connected to dirty symbols and add them to the dirty
+    // all nodes connected to dirty symbols and add them to the dirty node list.
-    // node list. This list now contains every node with a new or changed
+    // This list now contains every node with a new or changed graph hash.
-    // graph hash.
     //
     // We walk the dirty list and compute the new graph hashes, adding them
     // to the graph hash map. This Graph Map can then be passed to an actual
@@ -173,21 +173,15 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     $this->log(pht('BUILDING ATOM CACHE'));
 
     $file_hashes = $this->findFilesInProject();
-
     $this->log(pht('Found %d file(s) in project.', count($file_hashes)));
 
     $this->deleteDeadAtoms($file_hashes);
-
     $atomize = $this->getFilesToAtomize($file_hashes);
-
     $this->log(pht('Found %d unatomized, uncached file(s).', count($atomize)));
 
     $file_atomizers = $this->getAtomizersForFiles($atomize);
-
     $this->log(pht('Found %d file(s) to atomize.', count($file_atomizers)));
-
     $futures = $this->buildAtomizerFutures($file_atomizers);
-
     $this->log(pht('Atomizing %d file(s).', count($file_atomizers)));
 
     if ($futures) {
@@ -198,16 +192,13 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     }
 
     $this->log(pht('Writing atom cache.'));
-
     $this->getAtomCache()->saveAtoms();
-
     $this->log(pht('Done.')."\n");
   }
 
   private function getAtomizersForFiles(array $files) {
     $rules = $this->getRules();
     $exclude = $this->getExclude();
-
     $atomizers = array();
 
     foreach ($files as $file) {
@@ -221,7 +212,7 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
         $ok = preg_match($rule, $file);
         if ($ok === false) {
           throw new Exception(
-            "Rule '{$rule}' is not a valid regular expression.");
+            pht("Rule '%s' is not a valid regular expression.", $rule));
         }
         if ($ok) {
           $atomizers[$file] = $atomizer;
@@ -234,12 +225,10 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
   }
 
   private function getRules() {
-    $rules = $this->getConfig('rules', array(
+    return $this->getConfig('rules', array(
       '/\\.diviner$/' => 'DivinerArticleAtomizer',
       '/\\.php$/' => 'DivinerPHPAtomizer',
     ));
-
-    return $rules;
   }
 
   private function getExclude() {
@@ -247,7 +236,6 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     return $exclude;
   }
 
-
   private function findFilesInProject() {
     $raw_hashes = id(new FileFinder($this->getConfig('root')))
       ->excludePath('*/.*')
@@ -355,7 +343,6 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     $bar->done();
   }
 
-
   /**
    * Get a global version number, which changes whenever any atom or atomizer
    * implementation changes in a way which is not backward-compatible.
@@ -388,7 +375,6 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
 
 /* -(  Graph Cache  )-------------------------------------------------------- */
 
-
   private function buildGraphCache() {
     $this->log(pht('BUILDING GRAPH CACHE'));
 
@@ -401,6 +387,7 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
 
     $del_atoms = array_diff_key($symbol_map, $atoms);
     $this->log(pht('Found %d obsolete atom(s) in graph.', count($del_atoms)));
+
     foreach ($del_atoms as $nhash => $shash) {
       $atom_cache->deleteSymbol($nhash);
       $dirty_symbols[$shash] = true;
@@ -411,14 +398,13 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
 
     $new_atoms = array_diff_key($atoms, $symbol_map);
     $this->log(pht('Found %d new atom(s) in graph.', count($new_atoms)));
+
     foreach ($new_atoms as $nhash => $ignored) {
       $shash = $this->computeSymbolHash($nhash);
       $atom_cache->addSymbol($nhash, $shash);
       $dirty_symbols[$shash] = true;
 
-      $atom_cache->addEdges(
+      $atom_cache->addEdges($nhash, $this->getEdges($nhash));
-        $nhash,
-        $this->getEdges($nhash));
 
       $dirty_nhashes[$nhash] = true;
     }
@@ -467,7 +453,8 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     $atom = $atom_cache->getAtom($node_hash);
 
     if (!$atom) {
-      throw new Exception("No such atom with node hash '{$node_hash}'!");
+      throw new Exception(
+        pht("No such atom with node hash '%s'!", $node_hash));
     }
 
     $ref = DivinerAtomRef::newFromDictionary($atom['ref']);
@@ -481,7 +468,7 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     $refs = array();
 
     // Make the atom depend on its own symbol, so that all atoms with the same
-    // symbol are dirtied (e.g., if a codebase defines the function "f()"
+    // symbol are dirtied (e.g., if a codebase defines the function `f()`
     // several times, all of them should be dirtied when one is dirtied).
     $refs[DivinerAtomRef::newFromDictionary($atom)->toHash()] = true;
 
@@ -510,7 +497,6 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     return md5(serialize($inputs)).'G';
   }
 
-
   private function publishDocumentation($clean) {
     $atom_cache = $this->getAtomCache();
     $graph_map = $atom_cache->getGraphMap();
@@ -527,5 +513,4 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
     $this->log(pht('Done.'));
   }
 
-
 }

src/applications/diviner/workflow/DivinerWorkflow.php

@@ -20,15 +20,13 @@ abstract class DivinerWorkflow extends PhabricatorManagementWorkflow {
   protected function readBookConfiguration($book_path) {
     if ($book_path === null) {
       throw new PhutilArgumentUsageException(
-        'Specify a Diviner book configuration file with --book.');
+        pht(
+          'Specify a Diviner book configuration file with %s.',
+          '--book'));
     }
 
     $book_data = Filesystem::readFile($book_path);
-    $book = json_decode($book_data, true);
+    $book = phutil_json_decode($book_data);
-    if (!is_array($book)) {
-      throw new PhutilArgumentUsageException(
-        "Book configuration '{$book_path}' is not in JSON format.");
-    }
 
     PhutilTypeSpec::checkMap(
       $book,
@@ -55,8 +53,11 @@ abstract class DivinerWorkflow extends PhabricatorManagementWorkflow {
     if (!preg_match('/^[a-z][a-z-]*\z/', $book['name'])) {
       $name = $book['name'];
       throw new PhutilArgumentUsageException(
-        "Book configuration '{$book_path}' has name '{$name}', but book names ".
+        pht(
-        "must include only lowercase letters and hyphens.");
+          "Book configuration '%s' has name '%s', but book names must ".
+          "include only lowercase letters and hyphens.",
+          $book_path,
+          $name));
     }
 
     foreach (idx($book, 'groups', array()) as $group) {
@@ -66,7 +67,6 @@ abstract class DivinerWorkflow extends PhabricatorManagementWorkflow {
           'name' => 'string',
           'include' => 'optional regex|list<regex>',
         ));
-
     }
 
     $this->bookConfigPath = $book_path;