-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 18/11/2010 21:55, John A Meinel wrote: > 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'm being a bit picky here, but I'm hoping to provide some feedback to > make it easier to grow the code in the future. I certainly don't want to > turn you off about it. And some bits we could put comments of "this is > how we want it to be in the future", without having to do all of the > implementation up front. This is what I am looking for. You have raised some real issues, and given some good suggestions. Thanks. > '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 guess I called it *log*graphviz, because it is specific to then log graph, an not generic to any other graph, but it is quite log, so graph_viz sounds good. > 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). Ok > 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, No, but maybe loader is a bad name. The loader keeps a cache of what it loads. You may have more than state object per loader. In the case of loggerhead, the loader is cached, valid until then branch tip changes. For each http request, a new state object is created, with data parsed from the http query. Maybe the load method of the loader should be called by the loaders __init__ method. > 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=) > state = loader.process() > for node in state.iter_nodes(): > ... > > Basically, BranchInfo can be an implementation detail, rather than > something you create and-then-add. Currently, having the qlog create the BranchInfo reduces it's plumbing, because the BranchInfo are not created in then same place as then GraphVizLoader. I need to look at changing this. > + 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. I'm actually going to remove these. They no longer needed. > + """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. Yes - I was doing that when I did not know about the :ivar method of documenting variables. You will see then newer documentation is like that, and I need to change these. > + 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. key() in this case is a string, because then keys of the dict passed to the KnownGraph are strings. > + 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. Hmmm - At the moment, I have a mixin the implements these. A much better design would be to pass then view to then loader, and have then view implement these. That would be a much better design. > ... > + 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. Indeed. > 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. Good idea. I've come to dislike the inline functions. I don't think that moving this to it's own class will make it slower, as it will only be instantiating one instance of this object per compute. > Is there a reason PendingMergesGraphVizLoader and > WithWorkingTreeGraphVizLoader are separate classes? At most it seems > like configuration flags to a single loader object. When I first wrote them, the differences were more substantial. Through refactoring, I have reduced then differences. > +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. I had the # -*- coding: utf-8 -*- line in then qbzr copy. I removed by mistake when I changed the copyright. I restored the line shortly after I submitted the mp when I saw that the diff that launchpad sent was not encoded correctly. > But even accounting for that, I get: > +self.assertComputed( > + [(gv.tree_revid(new_tree), 2, None, [(2, 2, 3, True)]), # ○ > + # │ > + ('rev-new', 2, None, [(2, 2, 0, True)]), # ○ > + # │ > + (gv.tree_revid(old_tree), 1, None, [(1, 1, 2, True), # ○ │ > + (2, 2, 0, True)]), # │ │ > + ('rev-old', 1, None, [(1, 1, 0, True), (2, 2, 0, True)]), # ○ │ > + # │ │ > + (gv.tree_revid(trunk_tree), 0, None, [ # ○ │ │ > + (0, 0, 0, True), (1, 1, 0, True), (2, 2, 0, True)]), # │ │ │ > + ('rev-trunk', 0, None, [(0, 0, 0, True), (1, 0, 0, True), # ○ │ │ > + (2, 0, 0, True)]), # ├─╯─╯ > + ('rev-a', 0, None, [])], # ○ > + computed) > > Which doesnt seem a lot better than: > > # o > # | > # o > # o | > # | | > # o | > # | | > # o | | > # | | | > # o | | > # |/-' > # o > > Oh and gvim doesn't have ╯ as a character, it just renders it as a '[]' box. That does seem better. These ascii^H^H^H^H^H unicode art are created by the format_graph_lines method at the bottom of this file. It is also used when by assertComputed. It does have a option to output to ascii, but it is alot more difficult to read. Examples of this output: Ascii Unicode # * # ○ # | # │ # * | # ○ │ # | | # │ │ # * | | # ○ │ │ # |--- # ├─╯─╯ # * # ○ That ok. But when the graphs get a bit more complicated, it starts to fail. # + # ⊕ # |--- # ├┄╮─╮ # | : * # │ ┆ ○ # | -|- # │ ╰┄┼┄╮ # | | * # │ │ ○ # | | | # │ │ │ # ~ | | # ⊖ │ │ # |- | | # ├─╮ │ │ # | * | | # │ ○ │ │ # |----- # ├─╯─╯─╯ # * # ○ The unicode versions made it much easier for me to debug problems. > And I get some *really* strange results in whatever font Thunderbird > tries to set as default. Was that not because I left out the # -*- coding: utf-8 -*- line? Do the unicode characters show fine in this mail? > And all those lines are >80 chars as well. I did start trying to fit them in to < 80 chars, but this was time consuming, and for me, makes it harder to read. Here is a quote from PEP 8, which I feel applies: > Two good reasons to break a particular rule: > > (1) When applying the rule would make the code less readable, even for > someone who is used to reading code that follows the rules. Thanks alot for the review. You have give me lots of ideas on how to improve this. Gary -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkzmPswACgkQd/3EdwGKOh29VACeJaw3QjFlMSSY602FQw7VvAIn 7/kAoItKldupr3CUBENiL8i/nMERwpf/ =hsUh -----END PGP SIGNATURE-----