From ec39649449441af6b9d56eb048ac5ea0ff6fe8fc Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Sun, 1 Feb 2015 22:00:50 +1100 Subject: [PATCH] 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 --- .../workflow/DivinerAtomizeWorkflow.php | 26 +++--- .../workflow/DivinerGenerateWorkflow.php | 85 ++++++++----------- .../diviner/workflow/DivinerWorkflow.php | 18 ++-- 3 files changed, 58 insertions(+), 71 deletions(-) diff --git a/src/applications/diviner/workflow/DivinerAtomizeWorkflow.php b/src/applications/diviner/workflow/DivinerAtomizeWorkflow.php index 08c20a005c..c16cb24c30 100644 --- a/src/applications/diviner/workflow/DivinerAtomizeWorkflow.php +++ b/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 ". - "DivinerAtomizer."); + pht( + "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 = $json_encoder->encodeFormatted($all_atoms); + $json = id(new PhutilJSON())->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; } } diff --git a/src/applications/diviner/workflow/DivinerGenerateWorkflow.php b/src/applications/diviner/workflow/DivinerGenerateWorkflow.php index cc942e4df0..9e1370d73b 100644 --- a/src/applications/diviner/workflow/DivinerGenerateWorkflow.php +++ b/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 ". - "current directory. Use '--book ' to specify a ". - "documentation book to generate.")); + "There are no Diviner '%s' files anywhere beneath the current ". + "directory. Use '%s' to specify a documentation book to generate.", + '.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 - // - Atom Map: map + // - File Hash Map: `map` + // - Atom Map: `map` // // 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 - // 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 - // regenerate X. (In both these cases, the documentation for the connected - // atom may not acutally change, but in some cases it will, and the extra - // work we need to do is generally very small compared to the size of the - // project.) + // regenerated, because atoms depend on other atoms. For example, if `B + // 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 regenerate `X`. (In both these cases, the documentation for the + // connected atom may not actually change, but in some cases it will, and + // the extra work we need to do is generally very small compared to the + // 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 - // - Edge Map: map> - // - Graph Map: map + // - Symbol Map: `map` + // - Edge Map: `map>` + // - Graph Map: `map` // // 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 - // of DivinerAtomRefs. We can also build a symbolic reference for any atom - // from the atom itself. Each DivinerAtomRef generates a symbol hash, - // which ends with an "S", for "symbol". + // references to other nodes by name (e.g., `extends SomeClass`) in the form + // of @{class:DivinerAtomRefs}. We can also build a symbolic reference for + // any atom from the atom itself. Each @{class:DivinerAtomRef} generates a + // 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 - // find all nodes connected to dirty symbols and add them to the dirty - // node list. This list now contains every node with a new or changed - // graph hash. + // We initialize a list of dirty nodes to the list of new nodes, then find + // all nodes connected to dirty symbols and add them to the dirty node list. + // This list now contains every node with a new or changed 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( - $nhash, - $this->getEdges($nhash)); + $atom_cache->addEdges($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.')); } - } diff --git a/src/applications/diviner/workflow/DivinerWorkflow.php b/src/applications/diviner/workflow/DivinerWorkflow.php index 25b71760bd..152910a477 100644 --- a/src/applications/diviner/workflow/DivinerWorkflow.php +++ b/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); - if (!is_array($book)) { - throw new PhutilArgumentUsageException( - "Book configuration '{$book_path}' is not in JSON format."); - } + $book = phutil_json_decode($book_data); 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 ". - "must include only lowercase letters and hyphens."); + pht( + "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', )); - } $this->bookConfigPath = $book_path;