[guice] Graph private houcheng (#895)

[houcheng]

Add support to private module in graph extension.

- All elements in private module will be draw onto the result DOT file.
- Fix error in test.
- Note, the interface that used in higher level and defined in lower level
will be shown twice in the generated graph.

You can view, comment on, or merge this pull request online at:

https://github.com/google/guice/pull/895

-- Commit Summary --

* Add support to private module in graph extension
* Add support to private module in graph extension.

-- File Changes --

M extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java (50)
M extensions/grapher/src/com/google/inject/grapher/DefaultEdgeCreator.java (41)
M extensions/grapher/src/com/google/inject/grapher/DefaultNodeCreator.java (31)
A extensions/grapher/src/com/google/inject/grapher/DefaultSubgraphCreator.java (42)
M extensions/grapher/src/com/google/inject/grapher/EdgeCreator.java (2)
M extensions/grapher/src/com/google/inject/grapher/InjectorGrapher.java (3)
M extensions/grapher/src/com/google/inject/grapher/NodeCreator.java (2)
M extensions/grapher/src/com/google/inject/grapher/NodeId.java (17)
M extensions/grapher/src/com/google/inject/grapher/ProviderAliasCreator.java (4)
A extensions/grapher/src/com/google/inject/grapher/Subgraph.java (26)
A extensions/grapher/src/com/google/inject/grapher/SubgraphCreator.java (13)
M extensions/grapher/test/com/google/inject/grapher/AbstractInjectorGrapherTest.java (16)

-- Patch Links --

https://github.com/google/guice/pull/895.patch
https://github.com/google/guice/pull/895.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895

[houcheng]

The hierarchy graph sample is here:
Loading Image...


---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-71016711

[Sam Berlin]

@@ -38,6 +39,8 @@
private final AliasCreator aliasCreator;
private final NodeCreator nodeCreator;
private final EdgeCreator edgeCreator;
+ private final SubgraphCreator subCreator;

could you rename this variable to 'subgraphCreator' (and same below: foundSubs -> foundSubgraphs)? also, rm the extra space (there's two right now) btw the type & the var name.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23374994

[Sam Berlin]

@@ -38,6 +39,8 @@
private final AliasCreator aliasCreator;
private final NodeCreator nodeCreator;
private final EdgeCreator edgeCreator;
+ private final SubgraphCreator subCreator;
+ private final HashSet<Subgraph> foundSubs;

type the variable as Set<Subgraph>, not HashSet.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375010

[Sam Berlin]

@@ -45,7 +48,7 @@
private AliasCreator aliasCreator = new ProviderAliasCreator();
private NodeCreator nodeCreator = new DefaultNodeCreator();
private EdgeCreator edgeCreator = new DefaultEdgeCreator();
-
+ private SubgraphCreator subCreator = new DefaultSubgraphCreator();

rename same as above -- subCreator -> subgraphCreator.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375077

[Sam Berlin]

@@ -81,6 +84,14 @@ public GrapherParameters setEdgeCreator(EdgeCreator edgeCreator) {
this.edgeCreator = edgeCreator;
return this;
}
+
+ public SubgraphCreator getSubCreator() {

and throughout the file too: getSubCreator -> getSubgraphCreator, setSubCreator -> setSubgraphCreator.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375090

[Sam Berlin]

@@ -129,7 +147,7 @@ public AbstractInjectorGrapher(GrapherParameters options) {
/** Performs any post processing required after all nodes and edges have been added. */
protected abstract void postProcess() throws IOException;
- private void createNodes(Iterable<Node> nodes, Map<NodeId, NodeId> aliases) throws IOException {
+ private void createNodes(Iterable<Node> nodes, Map<NodeId, NodeId> aliases, int level) throws IOException {

can you add some javadoc describing what the params are for?

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375153

[Sam Berlin]

@@ -148,7 +166,7 @@ private void createNodes(Iterable<Node> nodes, Map<NodeId, NodeId> aliases) thro
}
}
- private void createEdges(Iterable<Edge> edges, Map<NodeId, NodeId> aliases) throws IOException {
+ private void createEdges(Iterable<Edge> edges, Map<NodeId, NodeId> aliases, int level) throws IOException {

also add some docs here.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375182

[Sam Berlin]

@@ -162,10 +180,28 @@ private void createEdges(Iterable<Edge> edges, Map<NodeId, NodeId> aliases) thro
}
}
+ private void appendSubs(Iterable<Subgraph> subs, Map<NodeId, NodeId> aliases) {

rename here too: appendSubs -> appendSubgraphs.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375208

[Sam Berlin]

@@ -162,10 +180,28 @@ private void createEdges(Iterable<Edge> edges, Map<NodeId, NodeId> aliases) thro
}
}
+ private void appendSubs(Iterable<Subgraph> subs, Map<NodeId, NodeId> aliases) {
+ for(Subgraph sub: subs) {

throughout the file please try to adhere to the google java style (see https://google-styleguide.googlecode.com/svn/trunk/javaguide.html) -- specifically, put a space after for/while and before the `(`. so: `for (Subgraph subgraph : subgraphs) {`

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375276

[Sam Berlin]

private NodeId resolveAlias(Map<NodeId, NodeId> aliases, NodeId nodeId) {
return aliases.containsKey(nodeId) ? aliases.get(nodeId) : nodeId;
}
+ private void createSubs() throws IOException {
+ Set<Key<?>> visitedKeys = Sets.newHashSet();
+ while(! foundSubs.isEmpty()) {

same style issue here: also rm the blank space between the `!` and the var.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375303

[Sam Berlin]

@@ -51,10 +51,13 @@
*/
private static final class NodeVisitor
extends DefaultBindingTargetVisitor<Object, Collection<Node>> {
-
+ String subname;

final

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375351

[Sam Berlin]

@@ -0,0 +1,42 @@
+package com.google.inject.grapher;

add apache license to the file.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375367

[Sam Berlin]

@@ -0,0 +1,42 @@
+package com.google.inject.grapher;
+
+import java.util.Collection;
+import java.util.List;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.google.inject.Binding;
+import com.google.inject.spi.DefaultBindingTargetVisitor;
+import com.google.inject.spi.ExposedBinding;
+
+/**
+ * Default subgraph creator.

this javadoc isn't terribly useful since it's just repeating the class name. instead, describe what the file does, why it does it, and maybe how it does it.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375399

[Sam Berlin]

+ *
+ */
+public class DefaultSubgraphCreator implements SubgraphCreator {
+ public Iterable<Subgraph> getSubs(Iterable<Binding<?>> bindings) {
+ List<Subgraph> subs = Lists.newArrayList();
+ SubgraphVisitor visitor = new SubgraphVisitor();
+ for (Binding<?> binding : bindings) {
+ subs.addAll(binding.acceptTargetVisitor(visitor));
+ }
+ return subs;
+ }
+}
+
+class SubgraphVisitor extends

looks like this inner class can be static.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375423

[Sam Berlin]

@@ -38,4 +40,5 @@
* their transitive dependencies.
*/
void graph(Injector injector, Set<Key<?>> root) throws IOException;
+ // List <Binding> getPrivates();

rm

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375452

[Sam Berlin]

@@ -0,0 +1,26 @@
+package com.google.inject.grapher;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.spi.ExposedBinding;
+
+/**
+ * Subgraph to represents a private module.
+ *
+ */
+public class Subgraph {

does this need to be a public API or can it be package-private?

also: all the variables should be private, fix the style (don't align the variables vertically, put a blank line btw the vars & the cxtor, no spaces between the type & the type parameters), make the vars final, add getters for the vars.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375565

[Sam Berlin]

@@ -0,0 +1,26 @@
+package com.google.inject.grapher;

add apache license.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375570

[Sam Berlin]

@@ -0,0 +1,13 @@
+package com.google.inject.grapher;
+
+import com.google.inject.Binding;
+
+/**
+ * Creator of subgraph.
+ *
+ */
+public interface SubgraphCreator {
+ /** Find subgraphs recursively for the given dependency graph. */
+ Iterable<Subgraph> getSubs(Iterable<Binding<?>> bindings);

make sure to rename this too. getSubs -> getSubgraphs.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375616

[Sam Berlin]

@@ -0,0 +1,13 @@
+package com.google.inject.grapher;
+
+import com.google.inject.Binding;
+
+/**
+ * Creator of subgraph.

elaborate here some more. explain what subgraphs are and how you'd find them.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895/files#r23375635

[Sam Berlin]

left a bunch of comments. mostly style issues, some API issues. please add tests too.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-71022318

[houcheng]

Hi, sameb:
Thanks for your review on my code, and I've modified all the issues accordingly.
Please see my latest commits in my branch. Should I close this request and
re-open another pull request?
Thanks a lot,
Houcheng LIn

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-71183039

[Sam Berlin]

No need to close & reopen, github pulls in new commits into existing PRs just fine. I'll take another look over this later today.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-71198385

[houcheng]

Hi, I added feature to support sub-graph cluster in generated graphviz graph.
The generated graph is like this:
Loading Image...


---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-73373578

[Nathan Howell]

Any chance this will get merged before the next release?

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-93611730

[Sam Berlin]

Sorry lost track of this. I'll do another review round soon with the aim of getting it mergable.

---
Reply to this email directly or view it on GitHub:
https://github.com/google/guice/pull/895#issuecomment-93612054