| === al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: -,- || queue: [sinzui, al-maisan<http://tinyurl.com/ycqtx3t>] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| al-maisan | intellectronica and henninge: when you open up shop please note that the branch in the queue fixes a release-critical bug. | 08:59 |
|---|---|---|
| al-maisan | henninge: when you open up shop please note that the branch in the queue fixes a release-critical bug | 09:29 |
| henninge | al-maisan: oh right, I am OCR today! ;) | 09:47 |
| === henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: -,- || queue: [sinzui, al-maisan<http://tinyurl.com/ycqtx3t>] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| al-maisan | henninge: :) | 09:47 |
| henninge | Shop's open. Please feel free to stroll around and look at the offerings, I'll be with you soon. | 09:48 |
| henninge | noodles775: I just requested an r-c review from you. Please feel free to ask me about it when you get to it. | 09:57 |
| noodles775 | henninge: will do. | 09:58 |
| === henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: al-maisan || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| henninge | al-maisan: The branch looks really good but I am wondering about test coverage. | 10:17 |
| al-maisan | henninge: aha | 10:17 |
| al-maisan | henninge: do you think it needs more/different testing? | 10:17 |
| henninge | al-maisan: I am just wondering if it would be possible to extend the test to test the case where the user has permission to upload to a series other than currentseries. | 10:19 |
| al-maisan | henninge: I can do that, no problem at all. | 10:19 |
| henninge | al-maisan: currently it only tests "allowed for currentseries, denied for others" | 10:19 |
| henninge | al-maisan: cool, I'd feel much better about that. | 10:19 |
| al-maisan | henninge: I'll ping you with the incremental diff in a few minutes. | 10:20 |
| henninge | al-maisan: did you already run the test suite on this? I am surprised that no other tests need to be adapted. | 10:20 |
| al-maisan | henninge: package sets are a pretty narrow niche .. but I will run the package upload tests as well .. thanks for pointing this out! | 10:21 |
| henninge | al-maisan: np | 10:24 |
| henninge | al-maisan: one last issue. I am not familiar with _schema_circular_imports.py but I think I see what it does. | 10:25 |
| al-maisan | henninge: it's the avoidance of circular imports.. | 10:26 |
| henninge | al-maisan: yes, so I gathered. I am just trying to figure out how it works. | 10:28 |
| henninge | al-maisan: I trust, though, that this is correct the way you did it, so no real issue for the review. | 10:28 |
| henninge | ;-) | 10:28 |
| al-maisan | the LP API metadata is manipulated afterwards out of the interface declaration scope | 10:29 |
| al-maisan | henninge: thanks | 10:29 |
| henninge | al-maisan: np, r=me | 10:33 |
| henninge | (what I meant) | 10:33 |
| al-maisan | henninge: thank you very much indeed. | 10:33 |
| === henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || (r-c requests skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| stub | henninge: https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14155 is an rc canditate | 11:50 |
| === henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: stub || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| al-maisan | hello henninge, I needed to tidy up a few things to get the requested tests to work: http://pastebin.ubuntu.com/307456/ | 11:52 |
| al-maisan | henninge: just want to make sure you approve of these changes as well | 11:52 |
| henninge | al-maisan: I understand | 11:53 |
| al-maisan | thanks | 11:53 |
| henninge | stub: I am onto yours next. | 11:53 |
| al-maisan | henninge: I also ran all package upload and archive related tests to make sure nothing breaks. | 11:53 |
| henninge | cool | 11:54 |
| henninge | al-maisan: so you had to make these changes just to be able to extend the test in this way? | 12:00 |
| al-maisan | henninge: yes, in order to manipulate archive permissions based on package sets, I needed to change the API to pass the package sets as opposed to their names. | 12:01 |
| al-maisan | that was the main change really. | 12:01 |
| henninge | al-maisan: btw, you seem to be using the API exclusively to test this. Is there a reason for that? | 12:03 |
| al-maisan | henninge: yes, this functionality is presently only exposed via the LP API. | 12:04 |
| henninge | al-maisan: ah | 12:04 |
| al-maisan | the UI is still to come | 12:04 |
| henninge | al-maisan: and now a packageset can only be passed in as an object, not represented by its name, as used to be the case, right? | 12:05 |
| al-maisan | henninge: yes, the reason being that package names are *not* globally unique any more | 12:05 |
| al-maisan | now, the combination of a package set name and a distro series is unique | 12:06 |
| al-maisan | s/package names/package set names/ | 12:06 |
| henninge | al-maisan: ah, I understand. | 12:07 |
| al-maisan | I could have added a 'distroseries' parameter but I thought it was more straight-forward to just pass in the package set *object* | 12:07 |
| henninge | al-maisan: but shouldn't there be some obsolete code now, that did the lookup from name to object? | 12:08 |
| al-maisan | henninge: let me check quickly | 12:09 |
| al-maisan | henninge: good catch, the ArchivePermission._nameToPackageset() method is now superfluous. | 12:11 |
| al-maisan | henninge: http://pastebin.ubuntu.com/307472/ | 12:13 |
| al-maisan | henninge: actually .. on a second thought .. scratch that .. the methods involved are internal (as in not exposed via the LP API) and it may still be OK to pass in package sets or package set names in this case. | 12:18 |
| al-maisan | the methods involved = the IArchivePermission methods in the most recent diff | 12:18 |
| al-maisan | henninge: hrm .. this is a bit complicated .. I guess one important aspect is that IArchivePermission methods are exposed *indirectly* via the IArchive LP API and that's where some of the confusion arose | 12:21 |
| henninge | al-maisan: sorry, team call atm. Won't be long. | 12:21 |
| al-maisan | henninge: cool. | 12:21 |
| henninge | al-maisan: so, your patch remains as it was, right? | 12:47 |
| al-maisan | henninge: yep | 12:48 |
| al-maisan | henninge: this change is still required: http://pastebin.ubuntu.com/307456/ | 12:49 |
| henninge | al-maisan: yeah, I believe you ... ;-) | 12:50 |
| henninge | al-maisan: and the new test looks very good! Thanks for adding that! | 12:50 |
| henninge | al-maisan: still r=me | 12:50 |
| al-maisan | henninge: thank you for prodding me :) | 12:50 |
| henninge | ;) | 12:50 |
| al-maisan | .. and thanks for the review | 12:50 |
| henninge | np | 12:50 |
| * henninge lunches | 12:53 | |
| === henninge is now known as henninge-lunch | ||
| === Ursinha-afk is now known as Ursinha | ||
| === noodles775_ is now known as noodles775 | ||
| === barry` is now known as barry | ||
| === bigjools is now known as bigjools-lunch | ||
| al-maisan | hello henninge-lunch, I have another release critical branch that needs a review: https://code.edge.launchpad.net/~al-maisan/launchpad/sync-470411/+merge/14301 | 13:44 |
| al-maisan | henninge-lunch: believe it or not but it's a 1-liner :) | 13:44 |
| === henninge-lunch is now known as henninge | ||
| henninge | al-maisan: done, r=me | 14:14 |
| al-maisan | henninge: thanks a million :) | 14:15 |
| henninge | al-maisan: nee, war doch nur *eine* Zeile! ;-) | 14:15 |
| al-maisan | aha :) | 14:15 |
| henninge | al-maisan: eine Million Zeilen hätte ich abgelehnt ... | 14:15 |
| al-maisan | henninge: ich hätte es Dir nicht 'mal übelgenommen :) | 14:16 |
| === abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === bigjools-lunch is now known as bigjools | ||
| === abentley changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub, - || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| wgrant | jml: Can you please review https://code.edge.launchpad.net/~wgrant/launchpad/distroseries-source-format-selection-db/+merge/14304? | 15:41 |
| === danilos changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: stub, - || (r-c candidates skip the queue) queue: [sinzui, danilo, danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| danilos | henninge, abentley: mine are not RC-candidates, so do give precedence to what is :) | 15:58 |
| === henninge changed the topic of #launchpad-reviews to: on call: henninge, abentley || reviewing: -, - || (r-c candidates skip the queue) queue: [sinzui, danilo, danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| henninge | danilos: I don't see why strip_last_newline is needed? | 16:24 |
| danilos | henninge, because gazillion of tests would have to be changed if it was something like rstrip("\r\n") | 16:25 |
| henninge | danilos: ;-) | 16:25 |
| danilos | henninge, well, not gazillion, but something like 4 very obscure tests, and if it was just .strip(), then something around 14 :) | 16:25 |
| henninge | danilos: ok, but my hunch was correct, that rstrip would be enough to do what you need! | 16:26 |
| danilos | henninge, also, .rstrip() strips all final newlines, and with some comments, whitespace should stay there | 16:26 |
| danilos | henninge, almost, I started with that, but tests correctly brought header comments to my attention, and they usually have a few newlines before the initial message, and I feel they belong there :) | 16:27 |
| danilos | henninge, I would have changed the few tests if it were not for these cases where we should keep extra newlines | 16:30 |
| henninge | danilos: sorry, phone call | 16:34 |
| danilos | henninge, was it for me? | 16:34 |
| henninge | danilos: ;-) | 16:35 |
| henninge | danilos: no, it was my dad | 16:35 |
| henninge | danilos: the other thing: strip_last_newline is testing for all versions of line breaks, but you only do split("\n") eventually. | 16:36 |
| danilos | henninge, oh, ok then :) | 16:36 |
| henninge | danilos: is there a magic in there that I forget that does line break type detection? | 16:37 |
| danilos | henninge, well, I don't have to fix all problems with that code? also, .split("\n") would work correctly with DOS-style line endings as well | 16:37 |
| danilos | henninge, and no, there isn't anything special that you are missing | 16:38 |
| henninge | >>> "Henning\r\nEggers".split("\n") | 16:38 |
| henninge | ['Henning\r', 'Eggers'] | 16:38 |
| danilos | henninge, see :) | 16:38 |
| henninge | danilos: if you are fine with that behaviour ... | 16:38 |
| danilos | henninge, now, you can insert "#" in front of 'Henning\r' and 'Eggers' and it'll be fine | 16:38 |
| danilos | henninge, we've got another set of bugs for dealing with \r's, but I am not touching those right now :) | 16:39 |
| henninge | danilos: ok, ok. r=me | 16:40 |
| === henninge changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || (r-c candidates skip the queue) queue: [sinzui, danilo, danilo] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| danilos | henninge, thanks | 16:42 |
| === deryck is now known as deryck[lunch] | ||
| === beuno is now known as beuno-lunch | ||
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || (r-c candidates skip the queue) queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: sinzui || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === deryck[lunch] is now known as deryck | ||
| === beuno-lunch is now known as beuno | ||
| sinzui | abentley: I have an rc-candidate to fix a problem with the product-release-finder: https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14311 | 18:03 |
| sinzui | oh, that is a bad diff | 18:04 |
| * sinzui deletes and tries again | 18:04 | |
| sinzui | abentley: RC candidate: https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14312 | 18:29 |
| === danilos is now known as danilos-afk | ||
| abentley | sinzui: r=me. | 19:38 |
| sinzui | thanks | 19:38 |
| abentley | sinzui: You should make two separate review requests, one for RC one for code. | 19:38 |
| sinzui | yes, you are right | 19:38 |
| sinzui | I added RC so that you would see it should hop in front of non-rc branches | 19:39 |
| abentley | sinzui: If you requested an RC review from flacoste, I would see that also. | 19:40 |
| sinzui | I see that I was not thinking | 19:40 |
| abentley | sinzui: Since I can't provide an "rc code" review, the Canonical Launchpad Engineering review request will never be satisfied. | 19:40 |
| flacoste | sinzui: do you have a r-c review request for me? | 19:41 |
| abentley | flacoste: We were discussing https://code.edge.launchpad.net/~sinzui/launchpad/prf-files/+merge/14312 | 19:42 |
| === EdwinGrubbs is now known as Edwin-lunch | ||
| === abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || (r-c candidates skip the queue) queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!