launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11881
[Merge] lp:~sinzui/launchpad/spec-unique-ids into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/spec-unique-ids into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #810664 in Launchpad itself: "Specification:+index asserts - (from_node, to_node) not in self.edges"
https://bugs.launchpad.net/launchpad/+bug/810664
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/spec-unique-ids/+merge/124040
OOPS-2021EE81 Obtained this oops after adding a blueprint as a
dependency. The graph code treated things as unique by their .name, when
we need something more sophisticated - like specification.target.name +
specification.name, or canonical_url(specification).
--------------------------------------------------------------------
RULES
Pre-implementation: jcsackett
* Update SpecGraphNode.name to be unique.
The name is used to generate dot-notation. We want to use a '-'
to separate the Specification.target.name from the Specification.name.
* Update SpecGraph.newNode() to raise a ValueError when the callsite
adds the wrong node.
* Update SpecGraph.getNode() to take the Specification as the argument.
QA
* Visit https://blueprints.qastaging.launchpad.net/gdp/+spec/new-root
* Verify the page loads (no tracekback)
* Verify the spec depends on two spec named gdplaunchpad.
LINT
lib/lp/blueprints/browser/specification.py
lib/lp/blueprints/doc/specgraph.txt
LOC
-15 lines saved by removing two redunant tests. Only the third
test was needed to demonstrate the graph was created properly.
TEST
./bin/test -vv -t specgraph lp.blueprints
IMPLEMENTATION
I Updated SpecGraphNode.name to use the target's name as a prefix, then
changed addNode() and getNode() to raise a value error if the spec was
already in the graph. The test was simple to write, but the unique-name
created a lot of mechanical changes. that verifies the graph's tree.
lib/lp/blueprints/browser/specification.py
lib/lp/blueprints/doc/specgraph.txt
--
https://code.launchpad.net/~sinzui/launchpad/spec-unique-ids/+merge/124040
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/spec-unique-ids into lp:launchpad.
=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py 2012-09-04 20:24:29 +0000
+++ lib/lp/blueprints/browser/specification.py 2012-09-12 19:28:36 +0000
@@ -1039,8 +1039,10 @@
There can be at most one root node set.
"""
- assert self.getNode(spec.name) is None, (
- "A spec called %s is already in the graph" % spec.name)
+ if self.getNode(spec) is not None:
+ raise ValueError(
+ "A spec called %s/%s is already in the graph" %
+ (spec.target.name, spec.name))
node = SpecGraphNode(spec, root=root,
url_pattern_for_testing=self.url_pattern_for_testing)
self.nodes.add(node)
@@ -1049,14 +1051,11 @@
self.root_node = node
return node
- def getNode(self, name):
- """Return the node with the given name.
-
- Return None if there is no such node.
- """
- # Efficiency: O(n)
+ def getNode(self, spec):
+ """Return the node with the given spec, or None if not matched."""
+ unique_name = '%s-%s' % (spec.target.name, spec.name)
for node in self.nodes:
- if node.name == name:
+ if node.name == unique_name:
return node
return None
@@ -1066,7 +1065,7 @@
If there is already a node for spec.name, return that node.
Otherwise, create a new node for the spec, and return that.
"""
- node = self.getNode(spec.name)
+ node = self.getNode(spec)
if node is None:
node = self.newNode(spec)
return node
@@ -1220,9 +1219,9 @@
"""
def __init__(self, spec, root=False, url_pattern_for_testing=None):
- self.name = spec.name
+ self.name = '%s-%s' % (spec.target.name, spec.name)
if url_pattern_for_testing:
- self.URL = url_pattern_for_testing % self.name
+ self.URL = url_pattern_for_testing % spec.name
else:
self.URL = canonical_url(spec)
self.isRoot = root
=== modified file 'lib/lp/blueprints/doc/specgraph.txt'
--- lib/lp/blueprints/doc/specgraph.txt 2011-12-24 17:49:30 +0000
+++ lib/lp/blueprints/doc/specgraph.txt 2012-09-12 19:28:36 +0000
@@ -16,12 +16,14 @@
>>> from lp.blueprints.browser.specification import SpecGraph
>>> g = SpecGraph()
>>> g.url_pattern_for_testing = 'http://whatever/%s'
+ >>> default_target = factory.makeProduct(name='fnord')
>>> class Spec(object):
...
... def __init__(self, name,
... is_complete=False, title=None, assignee=None):
... self.name = name
+ ... self.target = default_target
... self.title = title or name
... self.is_complete = is_complete
... self.assignee = assignee
@@ -36,13 +38,13 @@
>>> root = g.newNode(foo, root=True)
>>> print root
- <foo>
+ <fnord-foo>
Note that the root DOT data doesn't have a URL. This is because we
don't want a link to the spec we're currently looking at.
>>> print root.getDOTNodeStatement()
- "foo"
+ "fnord-foo"
[
"color"="red",
"comment"="something with \" and \n in it",
@@ -51,20 +53,21 @@
]
>>> print g.root_node
- <foo>
+ <fnord-foo>
>>> print root.name, root.label, root.URL, root.color
- foo foo http://whatever/foo red
+ fnord-foo foo http://whatever/foo red
- >>> g.getNode('no such name') is None
+ >>> other_spec = factory.makeSpecification()
+ >>> g.getNode(other_spec) is None
True
- >>> g.getNode('foo') is root
+ >>> g.getNode(foo) is root
True
>>> print g.listNodes()
- Root is <foo>
- <foo>:
+ Root is <fnord-foo>
+ <fnord-foo>:
>>> foo1 = Spec('foo1')
>>> foo.dependencies.append(foo1)
@@ -92,16 +95,16 @@
... print make_graph(dependency, blocked).getDOTGraphStatement()
>>> print_graph()
- Root is <foo>
- <foo>:
- <foo1>:
- foo
- <foo11>:
- foo1
- <foo111>:
- foo11
- <foo2>:
- foo
+ Root is <fnord-foo>
+ <fnord-foo>:
+ <fnord-foo1>:
+ fnord-foo
+ <fnord-foo11>:
+ fnord-foo1
+ <fnord-foo111>:
+ fnord-foo11
+ <fnord-foo2>:
+ fnord-foo
>>> print_graph_dot()
digraph "deptree" {
@@ -125,14 +128,14 @@
[
"arrowhead"="normal"
]
- "foo"
+ "fnord-foo"
[
"color"="red",
"comment"="something with \" and \n in it",
"label"="foo",
"tooltip"="something with \" and \n in it"
]
- "foo1"
+ "fnord-foo1"
[
"URL"="http://whatever/foo1";,
"color"="black",
@@ -140,7 +143,7 @@
"label"="foo1",
"tooltip"="foo1"
]
- "foo11"
+ "fnord-foo11"
[
"URL"="http://whatever/foo11";,
"color"="black",
@@ -148,7 +151,7 @@
"label"="foo11",
"tooltip"="foo11"
]
- "foo111"
+ "fnord-foo111"
[
"URL"="http://whatever/foo111";,
"color"="black",
@@ -156,7 +159,7 @@
"label"="foo111",
"tooltip"="foo111"
]
- "foo2"
+ "fnord-foo2"
[
"URL"="http://whatever/foo2";,
"color"="black",
@@ -164,87 +167,71 @@
"label"="foo2",
"tooltip"="foo2"
]
- "foo1" -> "foo"
- "foo11" -> "foo1"
- "foo111" -> "foo11"
- "foo2" -> "foo"
+ "fnord-foo1" -> "fnord-foo"
+ "fnord-foo11" -> "fnord-foo1"
+ "fnord-foo111" -> "fnord-foo11"
+ "fnord-foo2" -> "fnord-foo"
}
-Now, add a circle at the top.
+The graph grows when specifications gain more dependencies.
>>> foo1.dependencies.append(foo)
- >>> print_graph()
- Root is <foo>
- <foo>:
- foo1
- <foo1>:
- foo
- <foo11>:
- foo1
- <foo111>:
- foo11
- <foo2>:
- foo
-
-Now add another circle at the bottom.
-
>>> foo111.dependencies.append(foo1)
- >>> print_graph()
- Root is <foo>
- <foo>:
- foo1
- <foo1>:
- foo
- foo111
- <foo11>:
- foo1
- <foo111>:
- foo11
- <foo2>:
- foo
-
-Now make it even more convoluted, for fun.
-
>>> foo111.dependencies.append(foo)
>>> foo2.dependencies.append(foo1)
>>> foo1.dependencies.append(foo2)
>>> print_graph()
- Root is <foo>
- <foo>:
- foo1
- foo111
- <foo1>:
- foo
- foo111
- foo2
- <foo11>:
- foo1
- <foo111>:
- foo11
- <foo2>:
- foo
- foo1
+ Root is <fnord-foo>
+ <fnord-foo>:
+ fnord-foo1
+ fnord-foo111
+ <fnord-foo1>:
+ fnord-foo
+ fnord-foo111
+ fnord-foo2
+ <fnord-foo11>:
+ fnord-foo1
+ <fnord-foo111>:
+ fnord-foo11
+ <fnord-foo2>:
+ fnord-foo
+ fnord-foo1
And finally, try checking out the blocked specs too. Because of the
hack earlier, we have a "mirror image" of the dependencies in the
blocked speces.
>>> print_graph(dependency=False, blocked=True)
- Root is <foo>
- <foo>:
- foo1
- foo2
- <foo1>:
- foo
- foo11
- foo2
- <foo11>:
- foo111
- <foo111>:
- foo
- foo1
- <foo2>:
- foo1
+ Root is <fnord-foo>
+ <fnord-foo>:
+ fnord-foo1
+ fnord-foo2
+ <fnord-foo1>:
+ fnord-foo
+ fnord-foo11
+ fnord-foo2
+ <fnord-foo11>:
+ fnord-foo111
+ <fnord-foo111>:
+ fnord-foo
+ fnord-foo1
+ <fnord-foo2>:
+ fnord-foo1
+
+A spec with the same name, but from a different target can be a in
+the graph.
+
+ >>> test_graph = SpecGraph()
+ >>> ant_spec = factory.makeSpecification(name='ant')
+ >>> a_bat_spec = factory.makeSpecification(name='bat')
+ >>> b_bat_spec = factory.makeSpecification(name='bat')
+ >>> ignore = ant_spec.createDependency(a_bat_spec)
+ >>> ignore = ant_spec.createDependency(b_bat_spec)
+ >>> ant_node = test_graph.newNode(ant_spec, root=True)
+ >>> a_bat_node = test_graph.newNode(a_bat_spec)
+ >>> b_bat_node = test_graph.newNode(b_bat_spec)
+ >>> test_graph.getNode(b_bat_spec) is b_bat_node
+ True
SpecificationTreeImageTag and SpecificationView
Follow ups
