Code review - Issue 110049: Tryton download script in pythonhttps://codereview.appspot.com/2009-10-07T09:54:40+00:00rietveld
Message from unknown
2009-08-20T13:07:02+00:00yangoon1urn:md5:d867f68e1963e3c5cb3688325c4e1524
Message from unknown
2009-08-20T13:22:42+00:00yangoon1urn:md5:6e25ab1fbbc34bc4280656b189de77b5
Message from unknown
2009-08-20T13:25:38+00:00yangoon1urn:md5:c520b3bf0c90184e9465a12f9dc35750
Message from ced@b2ck.com
2009-08-20T13:44:06+00:00ced1urn:md5:54b1950a792361c5c49489e42bdd28d1
http://codereview.appspot.com/110049/diff/7/8
File tryton-dev (right):
http://codereview.appspot.com/110049/diff/7/8#newcode124
Line 124: modules.append(module)
You can use a set instead of a list
Message from unknown
2009-08-20T14:57:54+00:00yangoon1urn:md5:822733694ef4a05c7546beb18d4ecf4b
Message from h.goebel@goebel-consult.de
2009-08-22T14:17:39+00:00h.goebelurn:md5:772df98d4797bb4e61d374f66db386f8
http://codereview.appspot.com/110049/diff/1003/1004
File tryton-dev (right):
http://codereview.appspot.com/110049/diff/1003/1004#newcode122
Line 122: if module in modules:
Why should there be any need for check? Modules should be unique within each repository, thus this check would never trigger.
Duplicate download off modules is checked just 4 lines later and work across repositories.
Message from mathias.behrle@gmx.de
2009-08-22T22:11:33+00:00yangoon1urn:md5:d5ff56489ad8e3cd40648d3640384d5f
http://codereview.appspot.com/110049/diff/1003/1004
File tryton-dev (right):
http://codereview.appspot.com/110049/diff/1003/1004#newcode122
Line 122: if module in modules:
On 2009/08/22 14:17:39, h.goebel wrote:
> Why should there be any need for check? Modules should be unique within each
> repository, thus this check would never trigger.
> Duplicate download off modules is checked just 4 lines later and work across
> repositories.
The regular expression just finds all modules, in all branches. It is just not necessary to do needless clone trials and to have the message "already exists: ..." for duplicate module names on doing a completely fresh clone.
Message from h.goebel@goebel-consult.de
2009-08-25T13:54:06+00:00h.goebelurn:md5:d03b5559980adc3aa367d717e433844c
On 2009/08/22 22:11:33, yangoon wrote:
> The regular expression just finds all modules, in all branches. It is just not
> necessary to do needless clone trials and to have the message "already exists:
> ..." for duplicate module names on doing a completely fresh clone.
So the reg-exp is wrong and has to be corrected. No need for the 'modules'-set.
Message from unknown
2009-09-06T13:28:05+00:00yangoon1urn:md5:59d033a1ada2dd2a8c741f2e0f961490
Message from mathias.behrle@gmx.de
2009-09-06T13:28:06+00:00yangoon1urn:md5:335df47f4d67f9fc087dad92ea87583d
Message from unknown
2009-10-06T17:48:40+00:00yangoon1urn:md5:67b5c483effa5b50ac72bd79d7563372
Message from mathias.behrle@gmx.de
2009-10-06T17:48:41+00:00yangoon1urn:md5:567a448ffd4c3bce2c39586d8d0b0814
Message from unknown
2009-10-06T17:50:43+00:00yangoon1urn:md5:6e6ef5ed09761316fa983ba1a8f6d18e
Message from mathias.behrle@gmx.de
2009-10-06T17:50:44+00:00yangoon1urn:md5:e50ad5a7e57a380de3df2bbff9c74115
Message from h.goebel@goebel-consult.de
2009-10-07T07:35:31+00:00h.goebelurn:md5:24e5ae4f6686c8ed1f060065e6993974
I've tested the latest patches for both with and without branch and it works as expected. I suggest pushing the current version to roundup and replace old tryton-dev.sh
Message from mathias.behrle@gmx.de
2009-10-07T09:54:40+00:00yangoon1urn:md5:acd432b3fd4da5e8c9605b19ef8e9c2d
On 2009/10/07 07:35:31, h.goebel wrote:
> I've tested the latest patches for both with and without branch and it works as
> expected. I suggest pushing the current version to roundup and replace old
> tryton-dev.sh
I have tested this script, too, and it works ok now and provides extended functionality.
Nevertheless I would propose a different name like tryton-download.
I don' t recommend to replace the old script tryton-dev.sh, but to put it as alternative in the repos. This way manuals can refer to the one or the other and don't need to be fixed.
h.goebel: would you please provide the patch against repos tryton-dev and put it on https://bugs.tryton.org/roundup/issue1132?
Thx