/srv/irclogs.ubuntu.com/2010/03/03/#launchpad-reviews.txt

=== mwhudson_ is now known as mwhudson
=== daniloff is now known as danilos
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: ? || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisanhello allenap, I have a nice branch for you :)11:40
al-maisanhttps://code.edge.launchpad.net/~al-maisan/launchpad/ppa-visibility-514824/+merge/2053011:40
allenapal-maisan: Cool, I'll do that now.11:47
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: al-maisan || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisanthanks!11:47
=== matsubara-afk is now known as matsubara
allenapal-maisan: In xx-distribution-packages.txt, the disabled PPA is created, and it's absence from the PPA list is the test. Can you add a comment along the lines of "The disabled PPA is not shown in the page". Or, better, demonstrate that it is shown, disable it, and demonstrate that it is no longer shown.12:05
al-maisanallenap: I'll add the comment if you don't mind. Our test suite already takes too much time to run :)12:06
allenapal-maisan: Okay :)12:07
allenapal-maisan: Also, ViewSourcePackagePublishingHistory could be changed to just inherit from ViewArchive, with a custom __init__() that calls the superclass with obj.archive.12:08
al-maisanallenap: good point .. I'll look into that.12:08
allenapal-maisan: ViewArchive.checkAuthenticated() already checks for user.is_admin, so both ViewSourcePackagePublishingHistory.checkAuthenticated() and ViewSourcePackagePublishingHistory.checkUnauthenticated() both defer entirely to ViewArchive.12:09
al-maisanallenap: right.12:10
allenapal-maisan: You don't need to change that though; the composition is quite understandable.12:10
allenapal-maisan: I have to go and feed children, but I'll be back in <1h. Other than those observations, it all looks good :)12:10
al-maisanallenap: take care of the children and thanks for the review!12:11
adeuringallenap: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-511240-bug-heat-include-duplicate-bugs/+merge/20533 ? a real bargain: only 80 lines12:20
=== adeuring changed the topic of #launchpad-reviews to: on call: allenap || reviewing: al-maisan || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
bigjoolsal-maisan: I have a security fix, can I usurp the queue please :)13:05
bigjoolserr allenap, sorry13:05
allenapadeuring: Sure.13:13
allenapbigjools: Sure.13:13
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: al-maisan || queue [bigjools, adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== mrevell is now known as mrevell-lunch
allenapHey bigjools, where's your mp?13:15
allenapT'ain't in t'queue.13:15
bigjoolsallenap: I'm making a new one, the branch was for devel and the MP was for prod-devel so the diff was screwed13:15
allenapbigjools: Okeley dokely.13:16
* bigjools fights bazaar merging13:18
leonardrallenap, may i join the queue? (https://code.edge.launchpad.net/~leonardr/lazr.restful/refactor-tag-request-with-version/+merge/20547)13:52
allenapleonardr: Sure, go for it :)13:52
=== leonardr changed the topic of #launchpad-reviews to: on call: allenap || reviewing: al-maisan || queue [bigjools, adeuring, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisanhello allenap, here are the changes we discussed prior to the lunch break: http://pastebin.ubuntu.com/387623/14:23
allenapal-maisan: They look good. Push them and I'll add my r=me to the mp.14:25
al-maisanallenap: will do, thanks!14:25
al-maisanchanges pushed14:26
danilosadiroiban, btw, first things first: bug 525325 definitely doesn't belong in LP Foundations; I'd say it's either registry or translations, but that's a very minor point :)14:41
mupBug #525325: Export more ILanguage attributes in API <Launchpad Foundations:In Progress by adiroiban> <https://launchpad.net/bugs/525325>14:41
adiroibandanilos: ok. I asked on lp-dev about the code from services/worlddata and I was told it is part of foundations14:42
danilosadiroiban, hum, ok; I'd say nobody really knows what is it part of, but LP Translations team mostly maintains it (which is why I never bothered moving browser and other code out of lp.translations)14:47
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: adeuring || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleyallenap, could you please review https://code.edge.launchpad.net/~abentley/launchpad/qa-ready/+merge/20563 ?15:35
danilosadiroiban, hey, your branch is looking great, though I have a few questions :)15:35
allenapabentley: Sure, please add it to the queue. I won't get to it for a couple of hours because I have leonardr's branch to do and some groceries to get (my wife is too ill, still, to do that).15:36
danilosadiroiban, is there any reason not to use "only visible languages" for the default /languages call? and then provide an extra method that can return a subset of or all languages?15:36
=== abentley changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue [leonardr, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleyallenap, thanks.15:36
allenapleonardr: Actually, I'm not going to get to your branch either for an hour or so, due to grocery shopping. If it's urgent you might want to find another reviewer.15:36
allenapSorry.15:37
leonardrallenap, not too urgent15:37
allenapCool, I'll do both of those when I'm back.15:37
adiroibandanilos: the reason was to avoid exporting another `reallyGetAllLanguages` method15:38
adiroibanother than that, there is no reason. I can do that :)15:39
danilosadiroiban, well, I'd probably make it something like "getHiddenLanguages" or something; I am not sure myself what's the better pattern, so I'd like to discuss it before we settle on either way15:39
adiroibandanilos: then we can export only visible languages by default15:41
adiroibanand have a getAll operation that will get all languages15:41
danilosadiroiban, right, so how about a getVisibleLanguages as the default and getAllLanguages as another method?15:41
adiroibanI am not sure about the usage of getHiddenLangauges, but I guess it will always be used togheter with getVisibleLanguages15:42
adiroibanso getAll langauges can avoid a second call15:42
danilosadiroiban, though, will we ever have someone using API and not wanting hidden languages? for that matter, will we ever have anyone wanting to get all languages in the first place?15:42
adiroibandanilos: hidden languages can be used in Ubuntu to find our imported translations that diverge from the `main` language15:44
adiroibanother than that, I think most users will only care about visible languages15:44
adiroibandanilos: thus, I think we can just export all visible languages by default15:45
adiroibanand if someone ask15:45
adiroibanwe can add an operation to export all languages (including hidden one)15:45
adiroibanin the current lazr.restful implementation the default operation has no name, and it is available via +languages15:46
adiroibanto get all langauges, you will need to explicitly call an operation +languages?ws.op=getAll15:47
danilosadiroiban, right, I'd tend to agree15:47
adiroibandanilos: ok. then I will change the code to export only visible languages15:51
adiroibanother issues?15:51
danilosadiroiban, I'm still going through it, if any, it's going to be only minor issues15:57
danilosadiroiban, stylistic and such15:57
=== salgado is now known as salgado-lunch
noodles775Hi allenap, no probs. if you won't get to it, but I'll just pop this one on the queue: https://code.edge.launchpad.net/~michael.nelson/launchpad/522517-multiple-subscriptions-displayed/+merge/2056716:11
=== noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue [leonardr, abentley, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-lunch
bigjoolsallenap: replied to your review16:15
bigjoolsallenap: the branch page is showing a diff with stuff missing though :/16:16
bigjoolsand now it's there after I pushed a new revision.  Weird.16:17
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: - || queue [leonardr, abentley, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
EdwinGrubbsleonardr, abentley, noodles775: do any of you have urgent branches?16:41
abentleyEdwinGrubbs, no.16:41
leonardredwingrubbs, no16:41
noodles775EdwinGrubbs: no.16:41
EdwinGrubbsabentley: I'll take yours, since it looks like allenap said he would take leonardr's.16:43
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: abentley || queue [leonardr, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleyEdwinGrubbs, cool.16:45
leonardrallenap, Edwin, i've got another branch for the queue17:07
leonardrhttps://code.edge.launchpad.net/~leonardr/lazr.restful/make-test-class-public/+merge/2057417:07
=== leonardr changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: abentley || queue [leonardr, noodles775, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
EdwinGrubbsabentley: since I'm not really experienced in this part of the code, did you have a pre-impl call?17:08
allenapleonardr: I don't think I'll get to that, but I am about to start your first branch.17:08
abentleyEdwinGrubbs, no.17:08
EdwinGrubbsabentley: how do I go about testing it? I know how to create a mirrored branch, but do I need to run a cronscript to do the mirroring?17:09
abentleyEdwinGrubbs, that's not what we mean by mirror in this case.17:10
=== salgado-lunch is now known as salgado
abentleyYour mirrors are your local copy of stable and your local copy of db-stable.17:10
EdwinGrubbsabentley: ok, it's making a lot more sense now. I'll try running it.17:12
=== matsubara-lunch is now known as matsubara
=== allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue [noodles775, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
EdwinGrubbsabentley: I just want to confirm that this is the intended behavior:17:45
EdwinGrubbsMirror file:///home/egrubbs/canonical/lp-branches/db-stable/ is out of date.17:45
EdwinGrubbsDeployed on staging: False17:45
EdwinGrubbsabentley: when I updated the local mirror of db-stable, it then said True.17:45
abentleyEdwinGrubbs, that sounds reasonable, if your local copy of db-stable is out of date.17:45
abentleyUpdating your mirror of db-stable should not change the status, though.17:46
EdwinGrubbsabentley: is that because it does graph.is_ancestor() against the local_branch instead of the remote one?17:47
abentleyEdwinGrubbs, I assume it is, but that17:47
abentleyis strange, because I wil have just updated the local branch in that case.17:48
abentleyYou weren't running qa-ready against your db-stable branch itself, were you?17:48
EdwinGrubbsabentley: no.17:48
abentleyThat is baffling.17:49
EdwinGrubbsabentley: according to your cover letter, "It has no effect on17:49
EdwinGrubbsthe branch, only on what revisions are accessible in the branch's repository."17:49
abentleyEdwinGrubbs, correct.17:50
EdwinGrubbsabentley: so would that explain why graph.is_ancestor() can't find it in local_branch.last_revision()?17:50
abentleyEdwinGrubbs, hold up.17:51
abentleyIt appears to be because it does graph.is_ancestor against the local *mirror*, not the local branch.17:51
abentleyGraph traversal is a repository operation.17:53
abentleySo updating the repository of the local mirror should be sufficient.17:53
EdwinGrubbsabentley: are you going to be able to fix that easily?17:54
abentleySo pull in your mirror of db-stable should have no impact on graph traversal.17:54
abentleyEdwinGrubbs, I suspect that diagnosing the problem is going to be the most effort.17:55
abentleyAt this point, I still don't see how the behaviour you're reporting is possible.17:56
abentleyEdwinGrubbs, could you verify that the revno of your db-stable mirror matches the revno of db-stable itself?17:57
EdwinGrubbsabentley: I had pulled it before, and they are both at rev 9072 now.18:01
abentleyEdwinGrubbs, do you know what revno it was on before you pulled it?18:01
EdwinGrubbsabentley: this is interesting. If I uncommit the changes to my local db-stable branch and then run qa-ready, "Deployed on staging" is still True.18:02
EdwinGrubbsabentley: before I pulled it, it was several months old.18:02
danilosadiroiban, btw, it seems my review didn't get through via email; I've pasted it directly on the MP: only a few small bits left, it's a really great branch! thanks again :)18:02
abentleyEdwinGrubbs, when you uncommitted, you uncommitted back to where it was before?18:02
EdwinGrubbsabentley: no, I just uncommited till I got to the last revision where stable is merged into db-stable. "bzr missing" shows that the branch I'm running it on has one extra revision.18:04
=== jelmer_ is now known as jelmer
abentleyEdwinGrubbs, I'm trying to reproduce the issue locally.18:06
=== danilos is now known as daniloff
leonardredwin: another super small branch for the queue18:44
leonardrhttps://code.edge.launchpad.net/~leonardr/launchpadlib/multiversion-fix/+merge/2058518:44
=== leonardr changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue [noodles775, leonardr, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleyEdwinGrubbs, I was unable to reproduce the issue locally.  With a fresh branch of db-stable that was 50 revisions out of date, it still correctly determined the status of a branch that was deployed this morning.18:53
EdwinGrubbsabentley: I think that it is finding the revisions in the repository if they already are there, but the update of the repository during the script either doesn't update the repository correctly, or the script is ignoring the new data since the object were opened before that was added.18:56
abentleyEdwinGrubbs, that's why I used a fresh branch that doesn't have those revisions.18:57
EdwinGrubbsabentley: but isn't that branch using a shared repository?18:57
abentleyEdwinGrubbs, no, I created it as a standalone branch so that I could get an accurate test.18:57
EdwinGrubbsabentley: hmmm, that's weird18:58
abentleyEdwinGrubbs, it may have used the revisions from the shared repository via the local_branch, so I'm running another test.18:59
=== gary_poster is now known as gary-lunch
abentleyEdwinGrubbs, my second test also doesn't reproduce the problem.  I used a fresh copy of db-stable and the branch in question, and neither had the revision where the branch was merged.19:31
EdwinGrubbsabentley: ok, we'll assume it is a fluke, and wait for it to happen again. r=me19:32
abentleyEdwinGrubbs, thanks.19:32
=== EdwinGrubbs is now known as Edwin-lunch
=== jamalta-afk is now known as jamalta
=== gary-lunch is now known as gary_poster
leonardredwin, i'm leaving for the day. feel free to drop my branches from the queue if you find anything confusing20:38
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk
EdwinGrubbsleonardr: how do I run the tests.21:34
leonardrEdwinGribbs: which branch?21:34
leonardrpython bootstrap.py; bin/buildout; bin/test should work21:35
EdwinGrubbstrying it now21:41
sinzuiEdwinGrubbs: ping21:43
=== jamalta is now known as jamalta-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!