Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23503039:
Adding APISchemaGraph class to Extensions Docserver. (Closed)
|
Created:
7 years, 3 months ago by epeterson Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding APISchemaGraph class to Extensions Docserver.
A small step toward generating availability for APIs at the object level.
This class is used to convert API schema data to a more usable format for finding fine-grain availability information.
BUG=233982
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223362
Patch Set 1 #
Total comments: 21
Patch Set 2 : Small bugfix, Test improvement #Patch Set 3 : Changes. #
Total comments: 21
Patch Set 4 : Everything is better. #
Total comments: 6
Patch Set 5 : Significant changes. #
Total comments: 16
Patch Set 6 : Test Restructuring. #
Messages
Total messages: 16 (0 generated)
Hi Ben, Here's a more focused patch for object-level availability -- it's adding the AvailabilityGraph class. AvailabilityGraph takes in an API schema and a ChannelInfo object in __init__(), so it is designed to be created within each iteration of a HostFileSystemIterator's _ForEach() method. Or, it could be called by a higher-level wrapper that iterates over API schema data, as you've mentioned. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/availability_graph_test.py (right): https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:12: class AvailabilityGraphTest(unittest.TestCase): What do you think of this test? Terrifying? Wonderful? a little of both?
Oops, small bugfix with the test, I had added information to a fake data structure that wouldn't be there in an actual implementation. Also made the test clearer, and arguably less terrifying.
so yeah - tests should only go through interface methods (though sometimes you're forced to expose internals - but only sometimes, and hopefully reluctantly). The test I would like to see is based purely on: (1) construct a graph (2) Lookup() a bunch of things that are there, Lookup() a bunch of things that aren't. (3) subtract a graph from it (4) do the lookups again, check that it's correct. test subtraction with a few different configurations - empty graphs, subsets, supersets, disjoint sets. I'm not exactly sure where the channel info comes into it. I don't think it needs to; the users of the AvailabilityGraph can associate it themselves. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/availability_graph.py (right): https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:5: _ATTRIBUTES = ('events', 'functions', 'parameters', 'properties', 'types') I'm thinking this shouldn't care about what the contents is, it just attaches an availability to every object and know how to traverse. the _NameForNode method does still need to check, though. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:28: for node in api_schema: confusing dicts and lists here is.. odd. And I can see that _NameForNode tries to compensate by returning the node when it's passed a 'properties' node type. loop over them separately (check if dict or list, do the right thing). https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:29: name = _NameForNode(node, node_type) how about asserting "name not in graph" here. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:50: self.channel_info = channel_info these need to be private. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:91: return bool(self._paths) Would a better set of methods on this class be something like Subtract(self, other) which returns a new AvailabilityGraph? These SetPaths/UpdatePaths seem a little too implementation-detaily. Subtract would be some kind of combination of the two. That would let you attach the channel info to it. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/availability_graph_test.py (right): https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:12: class AvailabilityGraphTest(unittest.TestCase): On 2013/09/06 22:47:46, epeterson wrote: > What do you think of this test? > > Terrifying? Wonderful? > > > > a little of both? A little of both... half way there? Test interfaces, not implementations. Comparing against the internal availability_graph.graph is more trouble than it's worth. Just test with Lookup and make the test data tightly targeted. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:18: api_schema = [{ maybe put the test data at the top of the file so that it's easier to see where the test assertions are. share between the tests. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:66: expected_graph = { i.e. this test is not useful https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:166: self.assertEqual(availability_graph.Lookup(*path), all_channel_info[0]) but these are. you need these assertions with the test setup in testCreateGraph. no accessing availability_graph.graph. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:169: self.assertEqual(availability_graph.Lookup(*path), all_channel_info[3]) inline these |path|s though.
On 2013/09/06 23:29:37, kalman wrote: > so yeah - tests should only go through interface methods (though sometimes > you're forced to expose internals - but only sometimes, and hopefully > reluctantly). > > The test I would like to see is based purely on: > > (1) construct a graph > (2) Lookup() a bunch of things that are there, Lookup() a bunch of things that > aren't. > (3) subtract a graph from it > (4) do the lookups again, check that it's correct. > > test subtraction with a few different configurations - empty graphs, subsets, > supersets, disjoint sets. > > I'm not exactly sure where the channel info comes into it. I don't think it > needs to; the users of the AvailabilityGraph can associate it themselves. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > File chrome/common/extensions/docs/server2/availability_graph.py (right): > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph.py:5: _ATTRIBUTES = > ('events', 'functions', 'parameters', 'properties', 'types') > I'm thinking this shouldn't care about what the contents is, it just attaches an > availability to every object and know how to traverse. > > the _NameForNode method does still need to check, though. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph.py:28: for node in > api_schema: > confusing dicts and lists here is.. odd. And I can see that _NameForNode tries > to compensate by returning the node when it's passed a 'properties' node type. > loop over them separately (check if dict or list, do the right thing). > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph.py:29: name = > _NameForNode(node, node_type) > how about asserting "name not in graph" here. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph.py:50: > self.channel_info = channel_info > these need to be private. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph.py:91: return > bool(self._paths) > Would a better set of methods on this class be something like Subtract(self, > other) which returns a new AvailabilityGraph? These SetPaths/UpdatePaths seem a > little too implementation-detaily. Subtract would be some kind of combination of > the two. > > That would let you attach the channel info to it. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > File chrome/common/extensions/docs/server2/availability_graph_test.py (right): > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph_test.py:12: class > AvailabilityGraphTest(unittest.TestCase): > On 2013/09/06 22:47:46, epeterson wrote: > > What do you think of this test? > > > > Terrifying? Wonderful? > > > > > > > > a little of both? > > A little of both... half way there? Test interfaces, not implementations. > Comparing against the internal availability_graph.graph is more trouble than > it's worth. Just test with Lookup and make the test data tightly targeted. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph_test.py:18: api_schema > = [{ > maybe put the test data at the top of the file so that it's easier to see where > the test assertions are. share between the tests. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph_test.py:66: > expected_graph = { > i.e. this test is not useful > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph_test.py:166: > self.assertEqual(availability_graph.Lookup(*path), all_channel_info[0]) > but these are. you need these assertions with the test setup in testCreateGraph. > no accessing availability_graph.graph. > > https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... > chrome/common/extensions/docs/server2/availability_graph_test.py:169: > self.assertEqual(availability_graph.Lookup(*path), all_channel_info[3]) > inline these |path|s though. Renamed AvailabilityGraph to APISchemaMap, since it isn't really a 'graph' and it doesn't deal with availability anymore (currently, at least). The Subtract method could be used to find new parts of an API schema for a given version. Something like: 1. Find the difference between a trunk map and a version map 2. Find the difference between (1.) and the previous version map Or maybe just: 1. Find the difference between a version map and the previous version map This one might detect changes that are not currently available in trunk, though. I'm not quite sure yet how to assign availability information to nodes this way. I think this class might need to have another method to help with that. AvailabilityFinder could keep a reference to the map of new nodes added for each version, and then APIDataSource could send AvailabilityFinder a path to have each of its references Lookup(). But that seems ugly. Anyways, please see if this is closer to what you're looking for.
(Forgot to publish these) https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/availability_graph.py (right): https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:5: _ATTRIBUTES = ('events', 'functions', 'parameters', 'properties', 'types') On 2013/09/06 23:29:38, kalman wrote: > I'm thinking this shouldn't care about what the contents is, it just attaches an > availability to every object and know how to traverse. > > the _NameForNode method does still need to check, though. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:28: for node in api_schema: On 2013/09/06 23:29:38, kalman wrote: > confusing dicts and lists here is.. odd. And I can see that _NameForNode tries > to compensate by returning the node when it's passed a 'properties' node type. > loop over them separately (check if dict or list, do the right thing). Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:29: name = _NameForNode(node, node_type) On 2013/09/06 23:29:38, kalman wrote: > how about asserting "name not in graph" here. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:50: self.channel_info = channel_info On 2013/09/06 23:29:38, kalman wrote: > these need to be private. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph.py:91: return bool(self._paths) On 2013/09/06 23:29:38, kalman wrote: > Would a better set of methods on this class be something like Subtract(self, > other) which returns a new AvailabilityGraph? These SetPaths/UpdatePaths seem a > little too implementation-detaily. Subtract would be some kind of combination of > the two. > > That would let you attach the channel info to it. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/availability_graph_test.py (right): https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:12: class AvailabilityGraphTest(unittest.TestCase): On 2013/09/06 23:29:38, kalman wrote: > On 2013/09/06 22:47:46, epeterson wrote: > > What do you think of this test? > > > > Terrifying? Wonderful? > > > > > > > > a little of both? > > A little of both... half way there? Test interfaces, not implementations. > Comparing against the internal availability_graph.graph is more trouble than > it's worth. Just test with Lookup and make the test data tightly targeted. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:18: api_schema = [{ On 2013/09/06 23:29:38, kalman wrote: > maybe put the test data at the top of the file so that it's easier to see where > the test assertions are. share between the tests. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:66: expected_graph = { On 2013/09/06 23:29:38, kalman wrote: > i.e. this test is not useful Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:166: self.assertEqual(availability_graph.Lookup(*path), all_channel_info[0]) On 2013/09/06 23:29:38, kalman wrote: > but these are. you need these assertions with the test setup in testCreateGraph. > no accessing availability_graph.graph. Done. https://codereview.chromium.org/23503039/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/availability_graph_test.py:169: self.assertEqual(availability_graph.Lookup(*path), all_channel_info[3]) On 2013/09/06 23:29:38, kalman wrote: > inline these |path|s though. Done.
looking great. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:28: def _CreateMap(root, node_type=None): node_type is never used https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:38: api_schema_map[name] = {} Is/can this be written similar to: api_schema_map = dict(key, _CreateMap(value) for key, value in node.iteritems()) ? https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:56: def _Subtract(minuend, subtrahend): I've never heard the terms "minuend" and "subtrahend" before, but, cool. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:67: rest = _Subtract(minuend[key], subtrahend[key]) won't this break if minuend[key] or subtrahend[key] isn't a dict? https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:86: return APISchemaMap(_Subtract(self._map, other_map)) |other_map| should be an APISchemaMap, not... whatever it is. So I would expect this function call to look like _Subtract(self._map, other._map). (i.e. other_map -> other._map). https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:69: }] the structure is a little more complicated than it needs to be I think? Having all of types/functions/events/properties is good. However I don't thik they need to be quite that deep; you don't need to pass in totally valid values. E.g. you could replace 26 'functions': [ 27 { 28 'name': 'get', 29 'parameters': [ 30 { 31 'name': 'callback', 32 'parameters': [ 33 { 34 'name': 'tab' 35 } 36 ] 37 }, 38 { 39 'name': 'tabId' 40 } 41 ] 42 } 43 ], with 26 'functions': [ 27 { 28 'name': 'get', 29 'parameters': [ 'tab', 'tabId' ] 42 } 43 ], or whatever. that would make many of these tests just as efficient, but smaller and thus easier to follow. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:77: self.assertTrue(api_schema_map.Lookup('tabs')) Lookup should probably return the value there, or None if it's not found. I think that will end up being useful in the future, but if not, it's still nice. (I wouldn't worry in all cases to actually compare against that value if it's too complicated to do so; sometimes you could do != None). https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:117: # Test the same set. if it's the same set can't you subtract API_SCHEMA? https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:389: great tests
Changes have been made. Thank you for taking the time to review this patch. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:28: def _CreateMap(root, node_type=None): On 2013/09/12 20:34:23, kalman wrote: > node_type is never used Done. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:38: api_schema_map[name] = {} On 2013/09/12 20:34:23, kalman wrote: > Is/can this be written similar to: > > api_schema_map = dict(key, _CreateMap(value) for key, value in node.iteritems()) > > ? I do want that check, but I tried to make it neater, and used a comprehension because that's a cool idea. Without the check, something like the ('namespace', 'tabs') pair would end up in the dictionary as ('namespace', {}), which is useless here. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:56: def _Subtract(minuend, subtrahend): On 2013/09/12 20:34:23, kalman wrote: > I've never heard the terms "minuend" and "subtrahend" before, but, cool. I hadn't either. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:67: rest = _Subtract(minuend[key], subtrahend[key]) On 2013/09/12 20:34:23, kalman wrote: > won't this break if minuend[key] or subtrahend[key] isn't a dict? Updated the comment -- but the parameters here need to be 'maps', so there aren't any lists to worry about. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:86: return APISchemaMap(_Subtract(self._map, other_map)) On 2013/09/12 20:34:23, kalman wrote: > |other_map| should be an APISchemaMap, not... whatever it is. So I would expect > this function call to look like _Subtract(self._map, other._map). (i.e. > other_map -> other._map). Great, this makes things simpler for actually using this class in AvailabilityFinder. (Wasn't sure if I could access private member data for |other|) https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:69: }] On 2013/09/12 20:34:23, kalman wrote: > the structure is a little more complicated than it needs to be I think? Having > all of types/functions/events/properties is good. However I don't thik they need > to be quite that deep; you don't need to pass in totally valid values. E.g. you > could replace > > 26 'functions': [ > 27 { > 28 'name': 'get', > 29 'parameters': [ > 30 { > 31 'name': 'callback', > 32 'parameters': [ > 33 { > 34 'name': 'tab' > 35 } > 36 ] > 37 }, > 38 { > 39 'name': 'tabId' > 40 } > 41 ] > 42 } > 43 ], > > with > > 26 'functions': [ > 27 { > 28 'name': 'get', > 29 'parameters': [ 'tab', 'tabId' ] > 42 } > 43 ], > > or whatever. > > that would make many of these tests just as efficient, but smaller and thus > easier to follow. Done. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:77: self.assertTrue(api_schema_map.Lookup('tabs')) On 2013/09/12 20:34:23, kalman wrote: > Lookup should probably return the value there, or None if it's not found. I > think that will end up being useful in the future, but if not, it's still nice. > > (I wouldn't worry in all cases to actually compare against that value if it's > too complicated to do so; sometimes you could do != None). Done. Most of these checks have Lookup() traverse an entire path, so the returned result is just '{}'. Would you rather see shorter Lookup() paths so that there's actually some value to check against? https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:117: # Test the same set. On 2013/09/12 20:34:23, kalman wrote: > if it's the same set can't you subtract API_SCHEMA? Done. Changes made in APISchemaMap allow this now.
quick reply-reply. will look at new diff now. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:56: def _Subtract(minuend, subtrahend): On 2013/09/12 22:39:10, epeterson wrote: > On 2013/09/12 20:34:23, kalman wrote: > > I've never heard the terms "minuend" and "subtrahend" before, but, cool. > > I hadn't either. Btw another convention I see sometimes is to use "lhs" and "rhs". def _Subtract(self, lhs, rhs): pass but I like the way you have it. https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:67: rest = _Subtract(minuend[key], subtrahend[key]) On 2013/09/12 22:39:10, epeterson wrote: > On 2013/09/12 20:34:23, kalman wrote: > > won't this break if minuend[key] or subtrahend[key] isn't a dict? > > Updated the comment -- but the parameters here need to be 'maps', so there > aren't any lists to worry about. I'm.. not totally convinced. Why do they "need" to be maps? https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:86: return APISchemaMap(_Subtract(self._map, other_map)) On 2013/09/12 22:39:10, epeterson wrote: > (Wasn't sure if I could access private member data for > |other|) This is fine here because it's private data that this class has knowledge of. You'll notice in other languages like Java and C++ which enforce private via compilation that they will allow this too. Methods like isEqual are always implemented like that. public boolean isEqual(Object other) { return this.someValue == other.someValue && this.otherValue == other.someOtherValue; }
https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/11001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:67: rest = _Subtract(minuend[key], subtrahend[key]) On 2013/09/13 17:00:34, kalman wrote: > On 2013/09/12 22:39:10, epeterson wrote: > > On 2013/09/12 20:34:23, kalman wrote: > > > won't this break if minuend[key] or subtrahend[key] isn't a dict? > > > > Updated the comment -- but the parameters here need to be 'maps', so there > > aren't any lists to worry about. > > I'm.. not totally convinced. Why do they "need" to be maps? Ah, because you create |_map| yourself. Never mind.
lg apart from the comment in the test. https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:45: api_schema_map[name] = dict((key, _CreateMap(value)) for (key, value) key, value not (key, value) https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:49: api_schema_map[name] = dict((key, _CreateMap(value)) for (key, value) ditto https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:15: 'TAB_PROPERTY_TWO': {} not all values in the schema will resolve to maps; sometimes they will resolve to values, e.g. see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... this is why you need to test non-object values - and why it'd be useful to return the value at Lookup rather than just a bool.
We can now check the existence of 'non-object values.' https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:45: api_schema_map[name] = dict((key, _CreateMap(value)) for (key, value) On 2013/09/13 17:20:44, kalman wrote: > key, value not (key, value) Done. https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:49: api_schema_map[name] = dict((key, _CreateMap(value)) for (key, value) On 2013/09/13 17:20:44, kalman wrote: > ditto Done. https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/20001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:15: 'TAB_PROPERTY_TWO': {} On 2013/09/13 17:20:44, kalman wrote: > not all values in the schema will resolve to maps; sometimes they will resolve > to values, e.g. see > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > this is why you need to test non-object values - and why it'd be useful to > return the value at Lookup rather than just a bool. Done. It's all a little more complicated now, but the keys can be looked up.
just test comments now. Just needs a bit of restructuring. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:66: class APISchemaMap(object): I preferred Graph, and "graph" naming rather than "map". I wasn't thinking properly when you made that change, sorry. Map implies it's a map of something to API schema, which it's not. It's a graph representation of a schema. Or maybe it's a tree? https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:89: 'properties', 'two', 'value')) Pull all of these assertions into a different function... def testLookup(self): self._testApiSchema(APISchemaMap(API_SCHEMA)) def _testApiSchema(self, api_schema_map): self.assertTrue(...) https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:114: 'parameters', 'tab')) ... then call it from here. def testSubtractEmpty(self): self._testApiSchema(APISchemaMap(API_SCHEMA).Subtract(APISchemaMap({}))) https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:117: to_subtract = APISchemaMap(API_SCHEMA) ... and pull the rest into a different test (testSubtractSelf perhaps). https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:119: self.assertFalse(difference.Lookup('tabs')) Also this is one case where adding a specific method on APISchemaMap just for testing (... or at least, for now) would be nice, IsEmpty. This whole test should really be: def testIsEmpty(self): empty_map = APISchemaMap({}) self.assertTrue(empty_map.IsEmpty()) # A few assertions to make sure that Lookup works on empty sets. # NOTE TO EVAN: this is exactly the same checks you have already written here. self.assertFalse(...) self.assertFalse(...) def testSubtractSelf(self): self.assertTrue( APISchemaMap(API_SCHEMA).Subtract(APISchemaMap(API_SCHEMA)) .IsEmpty()) https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:130: # Test a disjoint set. separate test (testSubtractDisjointSet) https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:186: to_subtract = APISchemaMap({ separate test (testSubtractSubset) https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:235: to_subtract = APISchemaMap({ separate test (testSubtractSuperset)
Test has been organized a little more nicely. Also re-renamed APISchemaMap -> APISchemaGraph. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map.py (right): https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map.py:66: class APISchemaMap(object): On 2013/09/16 14:06:24, kalman wrote: > I preferred Graph, and "graph" naming rather than "map". I wasn't thinking > properly when you made that change, sorry. > > Map implies it's a map of something to API schema, which it's not. It's a graph > representation of a schema. Or maybe it's a tree? Done. I considered 'tree' earlier on but decided it looked a little silly. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_schema_map_test.py (right): https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:89: 'properties', 'two', 'value')) On 2013/09/16 14:06:24, kalman wrote: > Pull all of these assertions into a different function... > > def testLookup(self): > self._testApiSchema(APISchemaMap(API_SCHEMA)) > > def _testApiSchema(self, api_schema_map): > self.assertTrue(...) Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:114: 'parameters', 'tab')) On 2013/09/16 14:06:24, kalman wrote: > ... then call it from here. > > def testSubtractEmpty(self): > self._testApiSchema(APISchemaMap(API_SCHEMA).Subtract(APISchemaMap({}))) Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:117: to_subtract = APISchemaMap(API_SCHEMA) On 2013/09/16 14:06:24, kalman wrote: > ... and pull the rest into a different test (testSubtractSelf perhaps). Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:119: self.assertFalse(difference.Lookup('tabs')) On 2013/09/16 14:06:24, kalman wrote: > Also this is one case where adding a specific method on APISchemaMap just for > testing (... or at least, for now) would be nice, IsEmpty. This whole test > should really be: > > def testIsEmpty(self): > empty_map = APISchemaMap({}) > self.assertTrue(empty_map.IsEmpty()) > # A few assertions to make sure that Lookup works on empty sets. > # NOTE TO EVAN: this is exactly the same checks you have already written here. > self.assertFalse(...) > self.assertFalse(...) > > def testSubtractSelf(self): > self.assertTrue( > APISchemaMap(API_SCHEMA).Subtract(APISchemaMap(API_SCHEMA)) > .IsEmpty()) Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:130: # Test a disjoint set. On 2013/09/16 14:06:24, kalman wrote: > separate test (testSubtractDisjointSet) Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:186: to_subtract = APISchemaMap({ On 2013/09/16 14:06:24, kalman wrote: > separate test (testSubtractSubset) Done. https://codereview.chromium.org/23503039/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_schema_map_test.py:235: to_subtract = APISchemaMap({ On 2013/09/16 14:06:24, kalman wrote: > separate test (testSubtractSuperset) Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evan.peterson.EP@gmail.com/23503039/50001
Message was sent while issue was closed.
Change committed as 223362 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
