Update patch set 1
Patch Set 1: I would prefer that you didn't merge this (26 inline comments) Patch-set: 1 Label: Code-Review=-1
This commit is contained in:
Gerrit User 9107
Gerrit Code Review
parent
4b2c08cf9c
commit
0e444af86d
|
|
@ -1,5 +1,39 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vos\u003d",
|
||||
"filename": ".gitignore",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 18,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "why do you ignore these files? how are they generated?",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9voo\u003d",
|
||||
"filename": ".gitignore",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 21,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "tooz uses gettext?",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9v1I\u003d",
|
||||
|
|
@ -51,6 +85,23 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzo\u003d",
|
||||
"filename": "tooz/__init__.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 1,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "the encoding is uselss for empty files :)",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9v0o\u003d",
|
||||
|
|
@ -85,6 +136,23 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vyw\u003d",
|
||||
"filename": "tooz/api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 31,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "Why not writing **kwargs, or hosts\u003d...? Or (self, backend, hosts) if the parameter is mandatory.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9v0c\u003d",
|
||||
|
|
@ -102,6 +170,23 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vyU\u003d",
|
||||
"filename": "tooz/api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 44,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "copy.deepcopy() should be avoided when possible. If DriverManager modifies kwargs, DriverManager should copy the dictionary, but not this function.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vz8\u003d",
|
||||
|
|
@ -119,6 +204,23 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vyE\u003d",
|
||||
"filename": "tooz/api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 99,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "CoordinationDriver is the \"interface\" of API? If yes, why API is not a subclass of CoordinationDriver?",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vz0\u003d",
|
||||
|
|
@ -136,6 +238,91 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vx0\u003d",
|
||||
"filename": "tooz/drivers/__init__.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 3,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "coding and vim headers may be avoided in empty files",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vxk\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 19,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "you should add an empty line between external (kazoo) imports and internal (tooz) imports:\nhttp://docs.openstack.org/developer/hacking/#imports\n\nIf I understood OpenStack guide lines, you should only import symbols, Member is a class",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vwE\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 26,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "I don\u0027t know ZooKeeper, kazoo calls are blocking and may take a long time? If yes, do you know if there is an asynchronous API? It may be interesting to prepare the API for long blocking calls.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vuY\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 30,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "on error, I prefer to format values using %r. It\u0027s more readable and provides also the type.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vxE\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 33,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "member_id and client can be public attributes, \"we\u0027re all consenting adults here\" ;-)",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzk\u003d",
|
||||
|
|
@ -153,6 +340,40 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vuw\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 56,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "It may be interesting to modify this method to return a generator (yield), or maybe add a iter_members(). I don\u0027t know if there is an use case for an iterator.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vus\u003d",
|
||||
"filename": "tooz/drivers/zookeeper.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 79,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "just write return :-)",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzg\u003d",
|
||||
|
|
@ -170,6 +391,92 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vuA\u003d",
|
||||
"filename": "tooz/exceptions.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 19,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "Agreed.",
|
||||
"parentUuid": "AAAATn/9vzg\u003d",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vtg\u003d",
|
||||
"filename": "tooz/exceptions.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 20,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "I used such base class in the past, but now I think that it\u0027s bad practice. It\u0027s better to inherit from standard exceptions to have a better hierarchy of exceptions.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vtc\u003d",
|
||||
"filename": "tooz/exceptions.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 25,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "ex: inherit from ValueError... Or don\u0027t use your custom exception and simply raise ValueError.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vt4\u003d",
|
||||
"filename": "tooz/exceptions.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 26,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "PEP 8 requires an empty line here? (I prefer no empty line)",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vs8\u003d",
|
||||
"filename": "tooz/exceptions.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 31,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "ex: inherit from KeyError, or simply refuse KeyError\n\nIt sounds a little bit weird to me to read \"AlreadyExist\" in a class name, I would expect this information in the error message. Sorry, I don\u0027t have a better suggestion for the exception name.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzU\u003d",
|
||||
|
|
@ -187,6 +494,24 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vrY\u003d",
|
||||
"filename": "tooz/models.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 26,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "a \"bytes string\" to be more explicit\n\nI\u0027m not sure that Member is the best place to document capabilities.",
|
||||
"parentUuid": "AAAATn/9vzU\u003d",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzQ\u003d",
|
||||
|
|
@ -204,6 +529,24 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vr8\u003d",
|
||||
"filename": "tooz/models.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 34,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "Agreed, again, we are consentent adults ;)",
|
||||
"parentUuid": "AAAATn/9vzQ\u003d",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vzM\u003d",
|
||||
|
|
@ -220,6 +563,108 @@
|
|||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vpY\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 21,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "tooz import should be at the end, with a new line",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vpM\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 30,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "why do you need a zookeeper_tests variable?",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vqk\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 34,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "a backend may require a specific group or member identifier, you may add create_group_id() and create_member_id() methods (the default implementation would just call _get_random_uuid())\n\nyou may remove the \"_test\" suffix from those attributes.",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vq8\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 35,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "where does kwargs come from? From TestWithScenarios?",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vqM\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 48,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "reuse create_group_id() here if you chose to add such method",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "AAAATn/9vqE\u003d",
|
||||
"filename": "tooz/tests/test_api.py",
|
||||
"patchSetId": 1
|
||||
},
|
||||
"lineNbr": 66,
|
||||
"author": {
|
||||
"id": 9107
|
||||
},
|
||||
"writtenOn": "2013-12-06T13:44:55Z",
|
||||
"side": 1,
|
||||
"message": "member_ids name would be less confusing",
|
||||
"revId": "62e0248f00d4b07314d824a9711d8e045ea22e98",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in New Issue