Bazaar

Code review comment for lp:~garyvdm/bzr/loggraphviz

loggraphviz
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2010-11-18:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/18/2010 4:02 AM, Gary van der Merwe wrote:
> Gary van der Merwe has proposed merging lp:~garyvdm/bzr/loggraphviz into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This branch includes the loggraphviz module from qbzr, and is the module that implements the most of the logic for qlog, which I have refactored in the hopes that it may be used other plugins.
>
> I have implemented a proof of concept loggerhead viz view which use this. This is available here: lp:~garyvdm/loggerhead/loggraphviz
>
> There is still lots of work that I would like to do before this actually lands. E.g.:
> * The doc strings need to be completed, improved, and reviewed.
> * I want to have the api between the State objects, and Filters, especially how one adds a filter to a state more clearly defined.
> * I need to mark private/internal methods with _.
> * I want to do a bzr-gtk viz poc. viz has a number of features that qlog does not, i.e. progress bar, a revision command line argument to specify a tip., and a limit argument. Implementing these my result in api changes.
> * I would like to still investigate doing partial loading of merge sorted revisions using bzr-history-db for the use of loggerhead. This may also result in api changes.
>
> So I know this is not 100% ready, but I feel that as I have now done the loggerhead poc, I now feel the time is right to get some comments on the code.

To start off, I think the work you've done on graph visualization is
great, and I agree that it would be good to pull the core of that into
bzrlib itself.

'loggraphviz' is a bit clumsy for me. I don't have great ideas, but at
the very least it should be "log_graph_viz". I think calling it
"graph_visualization", or even just "graph_viz" would be reasonable.

I don't think you need to call each class GraphVizFoo inside the
loggraphviz module, though we've had discussions about that (without
strict resolution, afaict).

For naming, I would think that something called "Loader()" would return
the state object as part of .load(), rather than internalizing it. Your
example is:

+.. python::
+ bi = loggraphviz.BranchInfo('branch-label', tree, branch)
+ graph_viz = loggraphviz.GraphVizLoader([bi], bi, False)
+ graph_viz.load()
+ state = loggraphviz.GraphVizFilterState(graph_viz)
+ computed = graph_viz.compute_viz(state)

I would say,

1) Do we need the BranchInfo? Could it be done slightly differently?
loader = GraphVizLoader()
loader.add_branch('branch-label', branch)
# loader.add_tree() ?
# loader.add_branch('branch-label', branch, tree=<optional>)
state = loader.process()
for node in state.iter_nodes():
...

Basically, BranchInfo can be an implementation detail, rather than
something you create and-then-add.

+ def __eq__(self, other):
+ if isinstance(other, BranchInfo):
+ return self.branch.base.__eq__(other.branch.base)
+ return False

^- it seems odd that you don't check whether the label, presence of a
tree, or index are the same. I wonder if we really want to use "__eq__"
for this comparison, versus ".same_branch(other)"

Namely setting __hash__ and __eq__ in this way means that 2 BranchInfo
structures that have some differences are going to end up in the same
dictionary slot if you do my_info[bi] = X, my_info[bi2] = X, which may
or may not be what you really want.

+ """Revision indexes that this revision merges"""
+ self.merged_by = None
+ """Revision index that merges this revision."""
+ self.branch_id = self._merge_sort_node.revno[0:-1]

^- I realize this works (python ignores strings that aren't assigned),
but I'm pretty sure the bzr convention is to use comment lines, not strings.

If you really want to document them, then they should go in as:

:ivar merged_by: Revision indexes that this revision merges.

Oh, and you put the docstrings after the variable, whereas I think it
would be more common to put it before. All the more reason to put it in
as instance variable documentation in the class's docstring.

+ revid = property(lambda self: self._merge_sort_node.key)

^- .key() is a tuple [often StaticTuple], whereas revid would hint that
it is a simple string.

+ def get_revno_str(self):

^- probably reasonable to add '.revno_str()' as a property on the
underlying MergeSortNodes.

+class GraphVizLoader(object):
...
+ def update_ui(self):
+ pass
+
+ def throbber_show(self):
+ pass
+
+ def throbber_hide(self):
+ pass
+

^- These all seem related to 'View' code, while this is more of a Model
object.

...
+ if len(graph_parents) > 0:
+ enabled = gc.isenabled()
+ if enabled:
+ gc.disable()

^- I think I wrote this code, especially given the comments that follow.
:) Still, we probably want a try/finally around this, to help force the
state in case of an exception.

'make_kg()' only exists because of profiling limitations for lsprof (it
doesn't time the __init__ times for extension classes, so you need to
wrap that it a function it can profile.)

You have a lot of functions like "compute_viz" that make heavy use of
local variables. It would be clearer if we wrote that into a separate
class GraphVizComputer and made the shared state be class variables,
rather than local variables.

I realize it may be slower. But really, if it is a bottleneck in getting
stuff done, we should be going down to pyrex, rather than writing
hard-to-understand python code.

In general, 'compute_viz' is way too big. Not counting the inline
functions, it is 230 lines. I think it would be a lot easier to
understand as a collection of small functions.

Is there a reason PendingMergesGraphVizLoader and
WithWorkingTreeGraphVizLoader are separate classes? At most it seems
like configuration flags to a single loader object.

...

^- I'm pretty sure we try to avoid UTF-8 in our source code.

Especially since you don't have a -*- coding: utf-8 -*- line.

But even accounting for that, I get:
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzlgiAACgkQJdeBCYSNAAOSjgCdF2cwTs1ZaJzUPdn00STTXIUj
H5cAniDibMOCJ1CXII+TtyXPXNvwf8U/
=2ApL
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/18/2010 4:02 AM, Gary van der Merwe wrote:
> Gary van der Merwe has proposed merging lp:~garyvdm/bzr/loggraphviz into lp:bzr.
> 
> Requested reviews:
>   bzr-core (bzr-core)
> 
> 
> This branch includes the loggraphviz module from qbzr, and is the module that implements the most of the logic for qlog, which I have refactored in the hopes that it may be used other plugins.
> 
> I have implemented a proof of concept loggerhead viz view which use this. This is available here: lp:~garyvdm/loggerhead/loggraphviz
> 
> There is still lots of work that I would like to do before this actually lands. E.g.:
> * The doc strings need to be completed, improved, and reviewed.
> * I want to have the api between the State objects, and Filters, especially how one adds a filter to a state more clearly defined.
> * I need to mark private/internal methods with _.
> * I want to do a bzr-gtk viz poc. viz has a number of features that qlog does not, i.e. progress bar, a revision command line argument to specify a tip., and a limit argument. Implementing these my result in api changes.
> * I would like to still investigate doing partial loading of merge sorted revisions using bzr-history-db for the use of loggerhead. This may also result in api changes.
> 
> So I know this is not 100% ready, but I feel that as I have now done the loggerhead poc, I now feel the time is right to get some comments on the code.

To start off, I think the work you've done on graph visualization is
great, and I agree that it would be good to pull the core of that into
bzrlib itself.

I don't think you need to call each class GraphVizFoo inside the
loggraphviz module, though we've had discussions about that (without
strict resolution, afaict).

For naming, I would think that something called "Loader()" would return
the state object as part of .load(), rather than internalizing it. Your
example is:

+.. python::
+  bi = loggraphviz.BranchInfo('branch-label', tree, branch)
+  graph_viz = loggraphviz.GraphVizLoader([bi], bi, False)
+  graph_viz.load()
+  state = loggraphviz.GraphVizFilterState(graph_viz)
+  computed = graph_viz.compute_viz(state)

I would say,

1) Do we need the BranchInfo? Could it be done slightly differently?
 loader = GraphVizLoader()
 loader.add_branch('branch-label', branch)
 # loader.add_tree() ?
 # loader.add_branch('branch-label', branch, tree=<optional>)
 state = loader.process()
 for node in state.iter_nodes():
    ...

Basically, BranchInfo can be an implementation detail, rather than
something you create and-then-add.

+    def __eq__(self, other):
+        if isinstance(other, BranchInfo):
+            return self.branch.base.__eq__(other.branch.base)
+        return False

^- it seems odd that you don't check whether the label, presence of a
tree, or index are the same. I wonder if we really want to use "__eq__"
for this comparison, versus ".same_branch(other)"

+        """Revision indexes that this revision merges"""
+        self.merged_by = None
+        """Revision index that merges this revision."""
+        self.branch_id = self._merge_sort_node.revno[0:-1]

^- I realize this works (python ignores strings that aren't assigned),
but I'm pretty sure the bzr convention is to use comment lines, not strings.

If you really want to document them, then they should go in as:

:ivar merged_by: Revision indexes that this revision merges.

+    revid = property(lambda self: self._merge_sort_node.key)

^- .key() is a tuple [often StaticTuple], whereas revid would hint that
it is a simple string.

+    def get_revno_str(self):

^- probably reasonable to add '.revno_str()' as a property on the
underlying MergeSortNodes.

+class GraphVizLoader(object):
...
+    def update_ui(self):
+        pass
+
+    def throbber_show(self):
+        pass
+
+    def throbber_hide(self):
+        pass
+

^- These all seem related to 'View' code, while this is more of a Model
object.

...
+        if len(graph_parents) > 0:
+            enabled = gc.isenabled()
+            if enabled:
+                gc.disable()

^- I think I wrote this code, especially given the comments that follow.
:) Still, we probably want a try/finally around this, to help force the
state in case of an exception.

'make_kg()' only exists because of profiling limitations for lsprof (it
doesn't time the __init__ times for extension classes, so you need to
wrap that it a function it can profile.)

I realize it may be slower. But really, if it is a bottleneck in getting
stuff done, we should be going down to pyrex, rather than writing
hard-to-understand python code.

In general, 'compute_viz' is way too big. Not counting the inline
functions, it is 230 lines. I think it would be a lot easier to
understand as a collection of small functions.

Is there a reason PendingMergesGraphVizLoader and
WithWorkingTreeGraphVizLoader are separate classes? At most it seems
like configuration flags to a single loader object.

...

+        self.assertComputed(
+            [(u'current:%s' % tree.basedir, 0, True,                # âŠ–
+              [(0, 0, 0, True), (0, 1, 2, True)],                   #
â”œâ”€â•®
+              ['branch - Working Tree']),                           #
â”‚ â”‚
+             ('rev-b', 1, None, [(0, 0, 0, True), (1, 0, 0, True)], #
â”‚ â—‹
+              ['branch - Pending Merge']),                          #
â”œâ”€â•¯
+             ('rev-a', 0, None, [], ['branch'])],                   # â—‹
+            computed, branch_labels=True)

^- I'm pretty sure we try to avoid UTF-8 in our source code.

Especially since you don't have a -*- coding: utf-8 -*- line.

But even accounting for that, I get:
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzlgiAACgkQJdeBCYSNAAOSjgCdF2cwTs1ZaJzUPdn00STTXIUj
H5cAniDibMOCJ1CXII+TtyXPXNvwf8U/
=2ApL
-----END PGP SIGNATURE-----

« Back to merge proposal