[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.

[houcheng]

The hierarchy graph sample is here:
https://dl.dropboxusercontent.com/u/11742128/guice-graph.png

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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, not HashSet.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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?

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

> @@ -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) {

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/AbstractInjectorGrapher.java:

>    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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/DefaultNodeCreator.java:

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

final

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/DefaultSubgraphCreator.java:

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

add apache license to the file.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/DefaultSubgraphCreator.java:

> @@ -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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/DefaultSubgraphCreator.java:

> + *
> + * @author houc...@gmail.com (Houcheng Lin)
> + */
> +public class DefaultSubgraphCreator implements SubgraphCreator {
> +  @Override
> +  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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/InjectorGrapher.java:

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

rm

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/Subgraph.java:

> @@ -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.
> + *
> + * @author houc...@gmail.com (Houcheng Lin)
> + */
> +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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/Subgraph.java:

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

add apache license.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/SubgraphCreator.java:

> @@ -0,0 +1,13 @@
> +package com.google.inject.grapher;
> +
> +import com.google.inject.Binding;
> +
> +/**
> + * Creator of subgraph.
> + *
> + * @author houc...@gmail.com (Houcheng Lin)
> + */
> +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.

[Sam Berlin]

In extensions/grapher/src/com/google/inject/grapher/SubgraphCreator.java:

> @@ -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.

[Sam Berlin]

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

[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

[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.

[houcheng]

Hi, I added feature to support sub-graph cluster in generated graphviz graph.
The generated graph is like this:
https://dl.dropboxusercontent.com/u/11742128/guice-graph2.png

[Nathan Howell]

Any chance this will get merged before the next release?

[Sam Berlin]

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