From 730fe53dfa308ebfc8afa2fdf6342d1d588ed676 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Aug 2013 09:57:20 -0700 Subject: [PATCH] Move child loading into DivinerAtomQuery and collect/organize more data on class Atoms Summary: Ref T988. This is //extremely// rough looking in the UI, but gets most of the information we need into the right places. The controller rendering code is super rough too, I'm going to break that up shortly. - Add `needChildren()` to `DivinerAtomQuery`. - Compose and organize class methods when rendering classes. The old Diviner was not smart enough to do this, so a class would only document methods which the class itself implemented, not methods inherited from parents. I'd like to show those too to provide a more complete understanding of how to use a class (but they'll be marked with "inherited" or somesuch). This code walks the "extends" list and builds all of the class methods, annotating them with where they are defined and where they are implemented. - Coompose and organize "tasks". The old Diviner was not smart enough to do this, but I want to reduce the amount of duplicate/busy work involved in documenting subclasses. In particular, I want them to inherit "@task" declarations from parents so that class trees are more cohesive. They now do so. Test Plan: See screenshots. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T988 Differential Revision: https://secure.phabricator.com/D6823 --- .../controller/DivinerAtomController.php | 158 +++++++++++++++--- .../publisher/DivinerLivePublisher.php | 19 ++- .../diviner/query/DivinerAtomQuery.php | 87 +++++++++- .../diviner/storage/DivinerLiveSymbol.php | 11 ++ 4 files changed, 248 insertions(+), 27 deletions(-) diff --git a/src/applications/diviner/controller/DivinerAtomController.php b/src/applications/diviner/controller/DivinerAtomController.php index d381a53d03..d923d3d641 100644 --- a/src/applications/diviner/controller/DivinerAtomController.php +++ b/src/applications/diviner/controller/DivinerAtomController.php @@ -47,6 +47,7 @@ final class DivinerAtomController extends DivinerController { ->withIndexes(array($this->atomIndex)) ->needAtoms(true) ->needExtends(true) + ->needChildren(true) ->executeOne(); if (!$symbol) { @@ -54,20 +55,6 @@ final class DivinerAtomController extends DivinerController { } $atom = $symbol->getAtom(); - - $extends = $atom->getExtends(); - - $child_hashes = $atom->getChildHashes(); - if ($child_hashes) { - $children = id(new DivinerAtomQuery()) - ->setViewer($viewer) - ->withIncludeUndocumentable(true) - ->withNodeHashes($child_hashes) - ->execute(); - } else { - $children = array(); - } - $crumbs = $this->buildApplicationCrumbs(); $crumbs->addCrumb( @@ -170,14 +157,63 @@ final class DivinerAtomController extends DivinerController { ->setReturn($return)); } - if ($children) { + $methods = $this->composeMethods($symbol); + if ($methods) { + + $tasks = $this->composeTasks($symbol); + + if ($tasks) { + $methods_by_task = igroup($methods, 'task'); + + $document->appendChild( + id(new PhabricatorHeaderView()) + ->setHeader(pht('Tasks'))); + + if (isset($methods_by_task[''])) { + $tasks[''] = array( + 'name' => '', + 'title' => pht('Other Methods'), + 'defined' => $symbol, + ); + } + + foreach ($tasks as $spec) { + $document->appendChild( + id(new PhabricatorHeaderView()) + ->setHeader($spec['title'])); + + $task_methods = idx($methods_by_task, $spec['name'], array()); + if ($task_methods) { + $document->appendChild(hsprintf('')); + } else { + $document->appendChild("No methods for this task."); + } + } + } + $document->appendChild( id(new PhabricatorHeaderView()) ->setHeader(pht('Methods'))); - foreach ($children as $child) { - $document->appendChild( - id(new PhabricatorHeaderView()) - ->setHeader($child->getName())); + foreach ($methods as $spec) { + $method_header = id(new PhabricatorHeaderView()) + ->setHeader(last($spec['atoms'])->getName()); + + $inherited = $spec['inherited']; + if ($inherited) { + $method_header->addTag( + id(new PhabricatorTagView()) + ->setType(PhabricatorTagView::TYPE_STATE) + ->setBackgroundColor(PhabricatorTagView::COLOR_GREY) + ->setName(pht('Inherited'))); + } + + $document->appendChild($method_header); } } @@ -301,4 +337,88 @@ final class DivinerAtomController extends DivinerController { $view->addProperty(pht('Defined'), $defined); } + private function composeMethods(DivinerLiveSymbol $symbol) { + $methods = $this->findMethods($symbol); + if (!$methods) { + return $methods; + } + + foreach ($methods as $name => $method) { + // Check for "@task" on each parent, to find the most recently declared + // "@task". + $task = null; + foreach ($method['atoms'] as $key => $method_symbol) { + $atom = $method_symbol->getAtom(); + if ($atom->getDocblockMetaValue('task')) { + $task = $atom->getDocblockMetaValue('task'); + } + } + $methods[$name]['task'] = $task; + + // Set 'inherited' if this atom has no implementation of the method. + if (last($method['implementations']) !== $symbol) { + $methods[$name]['inherited'] = true; + } else { + $methods[$name]['inherited'] = false; + } + } + + return $methods; + } + + private function findMethods(DivinerLiveSymbol $symbol) { + $child_specs = array(); + foreach ($symbol->getExtends() as $extends) { + if ($extends->getType() == DivinerAtom::TYPE_CLASS) { + $child_specs = $this->findMethods($extends); + } + } + + foreach ($symbol->getChildren() as $child) { + if ($child->getType() == DivinerAtom::TYPE_METHOD) { + $name = $child->getName(); + if (isset($child_specs[$name])) { + $child_specs[$name]['atoms'][] = $child; + $child_specs[$name]['implementations'][] = $symbol; + } else { + $child_specs[$name] = array( + 'atoms' => array($child), + 'defined' => $symbol, + 'implementations' => array($symbol), + ); + } + } + } + + return $child_specs; + } + + private function composeTasks(DivinerLiveSymbol $symbol) { + $extends_task_specs = array(); + foreach ($symbol->getExtends() as $extends) { + $extends_task_specs += $this->composeTasks($extends); + } + + $task_specs = array(); + + $tasks = $symbol->getAtom()->getDocblockMetaValue('task'); + if (strlen($tasks)) { + $tasks = phutil_split_lines($tasks, $retain_endings = false); + + foreach ($tasks as $task) { + list($name, $title) = explode(' ', $task, 2); + $name = trim($name); + $title = trim($title); + + $task_specs[$name] = array( + 'name' => $name, + 'title' => $title, + 'defined' => $symbol, + ); + } + } + + return $task_specs + $extends_task_specs; + } + } diff --git a/src/applications/diviner/publisher/DivinerLivePublisher.php b/src/applications/diviner/publisher/DivinerLivePublisher.php index 164aa6f548..62004da063 100644 --- a/src/applications/diviner/publisher/DivinerLivePublisher.php +++ b/src/applications/diviner/publisher/DivinerLivePublisher.php @@ -126,12 +126,19 @@ final class DivinerLivePublisher extends DivinerPublisher { $symbol->save(); - if ($is_documentable) { - $storage = $this->loadAtomStorageForSymbol($symbol) - ->setAtomData($atom->toDictionary()) - ->setContent(null) - ->save(); - } + // TODO: We probably need a finer-grained sense of what "documentable" + // atoms are. Neither files nor methods are currently considered + // documentable, but for different reasons: files appear nowhere, while + // methods just don't appear at the top level. These are probably + // separate concepts. Since we need atoms in order to build method + // documentation, we insert them here. This also means we insert files, + // which are unnecessary and unused. Make sure this makes sense, but then + // probably introduce separate "isTopLevel" and "isDocumentable" flags? + + $storage = $this->loadAtomStorageForSymbol($symbol) + ->setAtomData($atom->toDictionary()) + ->setContent(null) + ->save(); } } diff --git a/src/applications/diviner/query/DivinerAtomQuery.php b/src/applications/diviner/query/DivinerAtomQuery.php index 06e6fda1e3..662bc834d8 100644 --- a/src/applications/diviner/query/DivinerAtomQuery.php +++ b/src/applications/diviner/query/DivinerAtomQuery.php @@ -16,6 +16,7 @@ final class DivinerAtomQuery private $needAtoms; private $needExtends; + private $needChildren; public function withIDs(array $ids) { $this->ids = $ids; @@ -62,6 +63,11 @@ final class DivinerAtomQuery return $this; } + public function needChildren($need) { + $this->needChildren = $need; + return $this; + } + /** * Include "ghosts", which are symbols which used to exist but do not exist @@ -130,8 +136,6 @@ final class DivinerAtomQuery $atom->attachBook($book); } - $need_atoms = $this->needAtoms; - if ($this->needAtoms) { $atom_data = id(new DivinerLiveAtom())->loadAllWhere( 'symbolPHID IN (%Ls)', @@ -170,6 +174,7 @@ final class DivinerAtomQuery ->withNames($names) ->needExtends(true) ->needAtoms(true) + ->needChildren($this->needChildren) ->execute(); $xatoms = mgroup($xatoms, 'getName', 'getType', 'getBookPHID'); } else { @@ -222,6 +227,25 @@ final class DivinerAtomQuery } } + if ($this->needChildren) { + $child_hashes = $this->getAllChildHashes($atoms, $this->needExtends); + + if ($child_hashes) { + $children = id(new DivinerAtomQuery()) + ->setViewer($this->getViewer()) + ->withIncludeUndocumentable(true) + ->withNodeHashes($child_hashes) + ->needAtoms($this->needAtoms) + ->execute(); + + $children = mpull($children, null, 'getNodeHash'); + } else { + $children = array(); + } + + $this->attachAllChildren($atoms, $children, $this->needExtends); + } + return $atoms; } @@ -322,4 +346,63 @@ final class DivinerAtomQuery return $this->formatWhereClause($where); } + + /** + * Walk a list of atoms and collect all the node hashes of the atoms' + * children. When recursing, also walk up the tree and collect children of + * atoms they extend. + * + * @param list List of symbols to collect child hashes of. + * @param bool True to collect children of extended atoms, + * as well. + * @return map Hashes of atoms' children. + */ + private function getAllChildHashes(array $symbols, $recurse_up) { + assert_instances_of($symbols, 'DivinerLiveSymbol'); + + $hashes = array(); + foreach ($symbols as $symbol) { + foreach ($symbol->getAtom()->getChildHashes() as $hash) { + $hashes[$hash] = $hash; + } + if ($recurse_up) { + $hashes += $this->getAllChildHashes($symbol->getExtends(), true); + } + } + + return $hashes; + } + + + /** + * Attach child atoms to existing atoms. In recursive mode, also attach child + * atoms to atoms that these atoms extend. + * + * @param list List of symbols to attach childeren to. + * @param map Map of symbols, keyed by node hash. + * @param bool True to attach children to extended atoms, as well. + * @return void + */ + private function attachAllChildren( + array $symbols, + array $children, + $recurse_up) { + + assert_instances_of($symbols, 'DivinerLiveSymbol'); + assert_instances_of($children, 'DivinerLiveSymbol'); + + foreach ($symbols as $symbol) { + $symbol_children = array(); + foreach ($symbol->getAtom()->getChildHashes() as $hash) { + if (isset($children[$hash])) { + $symbol_children[] = $children[$hash]; + } + } + $symbol->attachChildren($symbol_children); + if ($recurse_up) { + $this->attachAllChildren($symbol->getExtends(), $children, true); + } + } + } + } diff --git a/src/applications/diviner/storage/DivinerLiveSymbol.php b/src/applications/diviner/storage/DivinerLiveSymbol.php index 8a8a4bb79f..51bad9b94e 100644 --- a/src/applications/diviner/storage/DivinerLiveSymbol.php +++ b/src/applications/diviner/storage/DivinerLiveSymbol.php @@ -21,6 +21,7 @@ final class DivinerLiveSymbol extends DivinerDAO private $book = self::ATTACHABLE; private $atom = self::ATTACHABLE; private $extends = self::ATTACHABLE; + private $children = self::ATTACHABLE; public function getConfiguration() { return array( @@ -115,6 +116,16 @@ final class DivinerLiveSymbol extends DivinerDAO return $this->assertAttached($this->extends); } + public function attachChildren(array $children) { + assert_instances_of($children, 'DivinerLiveSymbol'); + $this->children = $children; + return $this; + } + + public function getChildren() { + return $this->assertAttached($this->children); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */