| abentley | intellectronica: I think it would be nice if windmill tests just pushed a config value that made canonical_url return the correct port and scheme. | 00:17 |
|---|---|---|
| intellectronica | abentley: yeah, or they could even just decorate canonical_url to do the transformation | 00:17 |
| abentley | That could work too. | 00:19 |
| intellectronica | abentley: actually, i think the best solution is to simply have another canonical_url in the testing package, and just import it from there. that's a bit more explicit than modifying the behaviour behind the user's back | 00:19 |
| abentley | intellectronica: The behaviour of canonical_url is already customized in a testing context-- I think it's not surprising. | 00:20 |
| intellectronica | you're right, i didn't consider that it's already modified | 00:21 |
| intellectronica | anyway, add that to the bug. it's too late for me to fix it now but i prolly will tomorrow morning | 00:21 |
| thumper | mwhudson: feel like reviewing my branch? | 02:32 |
| mwhudson | thumper: sure | 02:33 |
| * thumper sends it in | 02:34 | |
| intellectronica | who wants to review a windmill fix? it's a very easy review | 02:48 |
| intellectronica | gmb: awake? | 02:49 |
| intellectronica | mwhudson: review? it's pretty much just moving code around without change | 02:51 |
| mwhudson | intellectronica: ok | 02:51 |
| intellectronica | mwhudson: https://code.edge.launchpad.net/~intellectronica/launchpad/fix-test-mark-duplicate/+merge/13682 | 02:52 |
| intellectronica | mwhudson: http://pastebin.ubuntu.com/297931/ if you don't want to wait for the diff to generate | 02:53 |
| mwhudson | looking | 02:55 |
| intellectronica | mwhudson: all the patch does is take the test, move it to the directory up one level, and make the test function a method of a new proper test suite | 02:55 |
| mwhudson | intellectronica: done | 02:57 |
| intellectronica | mwhudson: danke | 02:58 |
| mwhudson | thumper: what's the difference between test_linked_to_development_package and test_linked_to_dev_package ? | 03:35 |
| thumper | ? | 03:35 |
| thumper | where? | 03:35 |
| mwhudson | thumper: in your branch, lib/lp/code/model/tests/test_branch.py | 03:37 |
| thumper | line number? | 03:37 |
| mwhudson | 489 & 506 | 03:37 |
| mwhudson | they look like different ways (yay, sigh) of saying the same thing | 03:38 |
| thumper | not quite | 03:38 |
| thumper | one uses the RELEASE pocket | 03:38 |
| thumper | so it is the dev focus | 03:38 |
| thumper | the other uses the backports pocket | 03:38 |
| thumper | so it definitely isn't | 03:38 |
| mwhudson | thumper: not that test | 03:39 |
| thumper | arse | 03:41 |
| thumper | it is | 03:41 |
| thumper | I thought I looked for a test that said that | 03:41 |
| thumper | grr | 03:41 |
| thumper | I can remove the duplicate | 03:41 |
| mwhudson | soyuz's confusatron field creeps into code :( | 03:42 |
| mwhudson | thumper: thanks | 03:42 |
| mwhudson | thumper: branch reviewed | 04:22 |
| thumper | thansk | 04:22 |
| thumper | aaaaaaaaaaaaaaaaaaaarrrrrrrrrrrrrrrrgggggggggggggggghhhhhhhhhhhhhhhhh | 04:24 |
| * thumper was reading wrong class | 04:24 | |
| mwhudson | thumper: ? | 04:24 |
| thumper | lazr-js TextAreaEditorWidget | 04:26 |
| thumper | mwhudson: I've fixed the accept_empty bit | 04:28 |
| thumper | mwhudson: and I can live with the style id as we don't yet have descriptions for merge proposals | 04:28 |
| thumper | ohh | 04:46 |
| thumper | bugger | 04:48 |
| mwhudson | thumper: it sounds like you're js hacking! | 04:49 |
| thumper | mwhudson: look at the priv msg | 04:49 |
| thumper | mwhudson: that is public | 04:49 |
| mwhudson | hm? | 04:49 |
| mwhudson | thumper: oh | 04:50 |
| mwhudson | bugger | 04:50 |
| thumper | yeah | 04:50 |
| * thumper is fixing | 04:50 | |
| thumper | mwhudson: there is a branch that simply moves the bmp registant ready :) | 05:01 |
| mwhudson | will loading the page do nasty things to my browser? | 05:02 |
| thumper | no | 05:02 |
| mwhudson | thumper: inline reviewing seems to have gone away | 05:03 |
| thumper | sometimes, and I don't know why | 05:03 |
| mwhudson | "yay" | 05:04 |
| thumper | mwhudson: any idea why this test doesn't fail? https://pastebin.canonical.com/23598/ | 05:29 |
| mwhudson | thumper: wouldn't that only fail after you've fixed the bug? | 05:32 |
| thumper | mwhudson: which I have | 05:32 |
| mwhudson | thumper: oh | 05:32 |
| mwhudson | then i don't know | 05:32 |
| thumper | mwhudson: I was changing the test to show that it is fixed | 05:32 |
| thumper | bum | 05:33 |
| mwhudson | maybe extract_text is full of crack? | 05:33 |
| thumper | mwhudson: the web ui shows the right thing | 05:33 |
| thumper | it shouldn't be | 05:33 |
| thumper | I wrote it :) | 05:33 |
| mwhudson | heh heh | 05:34 |
| thumper | mwhudson: I found it | 05:34 |
| thumper | mwhudson: I'm editing the wrong buffer | 05:34 |
| thumper | right file | 05:34 |
| thumper | wrong branch | 05:34 |
| thumper | D'oh | 05:34 |
| mwhudson | lolz | 05:34 |
| thumper | just did it again for the fix :( | 05:41 |
| thumper | mwhudson: want to review the v.small fix? | 05:46 |
| mwhudson | thumper: sure | 05:47 |
| thumper | mwhudson: just waiting on the diff | 05:49 |
| thumper | mwhudson: you should have it now | 05:51 |
| mwhudson | thumper: please get spm to cowboy the template fix? | 05:52 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: — || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: danilo || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| allenap | Morning jtv :) | 09:47 |
| jtv | allenap: morning! | 09:47 |
| jtv | allenap: oh, I already did danilo's. All the other ones on the queue were for me. :-) | 09:47 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: —, — || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| allenap | jtv: Okay, I'll do yours then. There's one from jelmer too, for launchpad-cscvs, but I think I might leave that for someone who's more familiar with vcs stuff than me. | 09:50 |
| jtv | allenap: I can try that one then. Hadn't seen it appear. | 09:50 |
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: —, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| adeuring | jtv: may I bother you with an MP having an overly long diff? 1000 lines, but mostly mechanical changes. Only ~130 lines are "real" changes. | 10:18 |
| jtv | adeuring: ok | 10:19 |
| adeuring | jtv: thanks! | 10:19 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: adeuring, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | adeuring: it's the new-attempt one? | 10:20 |
| adeuring | jtv: no, this branch: lp:~adeuring/launchpad/hwdb-build-udev-device-list-2 . Need to write the mp | 10:21 |
| adeuring | jtv: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-build-udev-device-list-2/+merge/13695/comments/35227/+reply | 10:33 |
| adeuring | erm, scrap that "+reply"; want to add the test command and the "make lint" output | 10:34 |
| jtv | adeuring: ok | 10:34 |
| adeuring | jtv: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-build-udev-device-list-2/+merge/13695 , that's what i menat. | 10:37 |
| jtv | adeuring: I had arrived there yes, thank you :) | 10:37 |
| jtv | adeuring: I do think it's a bit ugly to do self.devices = devices = {} | 10:43 |
| adeuring | jt: why? | 10:43 |
| jtv | The "shorthand" of saying devices instead of self.devices doesn't even save you much here. | 10:43 |
| adeuring | well it saves one word -- "self" ;) But I'll remove the local variable | 10:44 |
| jtv | When I read that line, I have to worry: do you really mean two references to the same dict, or is it a mistake? | 10:44 |
| jtv | Thanks. | 10:44 |
| jtv | adeuring: in the hwdb_submission_processing test, SubmissionParserFakedBuildDeviceList is a bit long as a name. How about MockSubmissionParser? | 10:48 |
| adeuring | jtv: Sure | 10:48 |
| jtv | Good testing technique, btw. I sometimes use the real object and just inject a callable mock object in place of a method, but if you can get away with something short and sweet like this, great. | 10:49 |
| adeuring | jtv: the problem with mock ojbects here is that it such data would probably look quite ugly and convoluted. The test file has many examples of this ;) | 10:51 |
| jtv | adeuring: in test_processSubmission_buildDeviceList_failing it might be useful though. | 10:53 |
| adeuring | jtwhy? | 10:53 |
| jtv | def no(*args, **kwargs): | 10:53 |
| jtv | return False | 10:53 |
| jtv | #... | 10:53 |
| jtv | parser.buildDeviceList = no | 10:53 |
| jtv | adeuring: that saves you a lot of long identifiers (though "no" may be a bit too short :-) | 10:54 |
| adeuring | jtv: question is what sort of test styles we prefer. I another review I got the recommendation to avoid monkey-patching ;) | 10:55 |
| adeuring | jtv: personally, I don't mind much, btu we should be consistent ;) | 10:56 |
| jtv | adeuring: IMHO, only when there's a clear winner among the options. :-) | 10:56 |
| adeuring | jtv: well, as I said, I really don't mind. So, you think I should use a method like "nyet(...)"? | 10:57 |
| adeuring | ...oder "nein()" | 10:57 |
| jtv | adeuring: in this case I think it's justified. | 10:58 |
| adeuring | jtv: OK, changed..... | 11:00 |
| jtv | adeuring: approved | 11:02 |
| adeuring | jtv: thanks! | 11:02 |
| jtv | macht nix | 11:02 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: —, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| leonardr | jtv, allenap: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/264098-eq-ne/+merge/13697 | 12:08 |
| allenap | leonardr, jtv: I can take that. | 12:10 |
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: —, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === matsubara-afk is now known as matsubara | ||
| jtv | allenap: thanks, some very nice suggestions there. And the one hard-to-document parameter turned out to be unused. :) | 12:24 |
| allenap | jtv: Woohoo :) | 12:25 |
| allenap | leonardr: You have tests for recipe.__hash__(), but there is nothing in the diff that defines the __hash__ method. | 12:38 |
| leonardr | allenap: i'm using the default python object implementation | 12:39 |
| leonardr | it was the only way i could think of to show that these two python objects were different | 12:39 |
| leonardr | but i'm probably missing something obvious | 12:39 |
| allenap | leonardr: Okay. You could say recipe is recipe2 and expect False. | 12:39 |
| leonardr | ok | 12:39 |
| leonardr | that's nice and obvious | 12:39 |
| danilos | jtv: got another one for you ;) https://code.edge.launchpad.net/~danilo/launchpad/bug-455771/+merge/13699 (diff should be regenerated any minute now) | 12:40 |
| jtv | give them a finger and they want the whole hand... | 12:40 |
| danilos | jtv: that's how it works :) | 12:46 |
| danilos | jtv: anyway, the diff is there now | 12:46 |
| jtv | sigh. Coming... | 12:46 |
| jtv | (for the public record: not serious :) | 12:46 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | danilos: I'm too lazy to look it up, but you have a conditional top-portlet now. Won't that upset your layout when the div is not shown? | 12:50 |
| === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: danilo, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| danilos | jtv: nope | 13:06 |
| jtv | danilos: too late; it's already in the mp. :-P | 13:07 |
| leonardr | jtv, allenap: and now, https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/verbose-http-error/+merge/13702 | 13:16 |
| jtv | leonardr: sorry, I'm just eod'ing (and in standup) | 13:16 |
| leonardr | np | 13:17 |
| === jtv changed the topic of #launchpad-reviews to: on call: allenap || reviewing: — || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | allenap: bowing out. Have a good one! | 13:20 |
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| leonardr | allenap, can you look at the branch i linked above? | 13:50 |
| === jamalta_ is now known as jamalta | ||
| === gary_poster is now known as gary_poster-spri | ||
| === gary_poster-spri is now known as gary-sprint | ||
| === abentley1 is now known as abentley | ||
| allenap | jtv: Cheerio :) I'll review your bug #417968 branch now. | 15:00 |
| mup | Bug #417968: Auto-approver: pathless template with unique name <import-queue> <Launchpad Translations:In Progress by jtv> <https://launchpad.net/bugs/417968> | 15:00 |
| === allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === salgado is now known as salgado-sprint | ||
| leonardr | allenap, edwin: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-media-type-misspelling/+merge/13711 | 15:57 |
| allenap | leonardr: I'm on it. | 15:59 |
| === allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv&leonardr, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| allenap | leonardr: r=me | 16:07 |
| === allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: jtv, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === salgado is now known as salgado-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| leonardr | allenap: and the follow-up: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/fix-media-type-misspelling/+merge/13715 | 16:44 |
| allenap | leonardr: Okay. | 16:44 |
| === allenap changed the topic of #launchpad-reviews to: on call: allenap, Edwin || reviewing: leonardr, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| allenap | EdwinGrubbs: Do you have much VCS-fu? There's a proposal from Jelmer for launchpad-cscvs, https://code.edge.launchpad.net/~jelmer/launchpad-cscvs/converted-from/+merge/13542 | 16:46 |
| jml | allenap, Were I you I'd politely ask abentley to review that one. | 16:51 |
| allenap | jml: Okay. | 16:52 |
| allenap | abentley: Please can you have a look at Jelmer's launchpad-cscvs proposal, https://code.edge.launchpad.net/~jelmer/launchpad-cscvs/converted-from/+merge/13542 :) | 16:53 |
| abentley | jml, allenap: rockstar volunteers. | 16:53 |
| rockstar | allenap, I can take it. I'm one of the few who have actually braved cscvs. | 16:53 |
| rockstar | And I like abentley too much to expose him to that. | 16:53 |
| allenap | rockstar: Awesome :) | 16:53 |
| jml | rockstar, yay | 16:54 |
| allenap | leonardr: r=me, with a question/suggestion. | 16:59 |
| === allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| leonardr | that's not a bad suggestion | 17:00 |
| === deryck is now known as deryck[lunch] | ||
| === gary-sprint is now known as gary-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === salgado is now known as salgado-sprint | ||
| === matsubara-lunch is now known as matsubara | ||
| EdwinGrubbs | allenap: I don't think I'm qualified to review that. | 17:15 |
| allenap | EdwinGrubbs: Sorry, rockstar took it already, shortly after I asked you. | 17:17 |
| allenap | EdwinGrubbs: I should have said something :( | 17:17 |
| EdwinGrubbs | allenap: well, I was having networking issues anyway. | 17:17 |
| === beuno is now known as beuno-lunch | ||
| === gary-lunch is now known as gary-sprint | ||
| leonardr | jml, are you around? | 17:57 |
| jml | leonardr, yes. | 17:58 |
| jml | leonardr, how can I help? | 17:58 |
| leonardr | i'd like your opinion on a branch i've written to address bug 319432 | 17:58 |
| mup | Bug #319432: RelativeURIError when trying to access an redacted object <api> <tech-debt> <launchpadlib :Triaged by leonardr> <https://launchpad.net/bugs/319432> | 17:58 |
| leonardr | i'm writing an mp now | 17:59 |
| jml | leonardr, my opinion is always available :) | 18:00 |
| === deryck[lunch] is now known as deryck | ||
| leonardr | jml: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/319432-redacted/+merge/13716 | 18:03 |
| === beuno-lunch is now known as beuno | ||
| === leonardr is now known as leonardr-lunch | ||
| === maxb_ is now known as maxb | ||
| === leonardr-lunch is now known as leonardr | ||
| leonardr | edwin, would you look at https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/319432-redacted/+merge/13716 ? | 19:06 |
| bac` | beuno: you around for some ui questions? | 19:52 |
| beuno | bac`, sure | 19:53 |
| bac` | beuno: great. i'm working on bug 419742. (hey i like my jaunty little reverse grave.) | 19:53 |
| mup | Bug #419742: downloads page should include some data about the releases <post-3-ui-cleanup> <Launchpad Registry:In Progress by bac> <https://launchpad.net/bugs/419742> | 19:53 |
| bac` | salgado had made some changes coming up with http://launchpadlibrarian.net/30906325/download.png | 19:54 |
| bac` | i've been trying to work in poolie's other comments and have gotten this: | 19:55 |
| bac` | http://people.canonical.com/~bac/download.png | 19:55 |
| bac` | beuno: i was hoping you could give me some ideas if i'm on the right track | 19:55 |
| * beuno looks | 19:56 | |
| bac` | beuno: i've made the 'release notes' optional, added 'changelog' and made it optional and collapsible, and moved the date out there to the right. | 19:57 |
| bac` | oh, added more vertical separation between the sections | 19:57 |
| beuno | vertical separation == awesome | 19:58 |
| bac` | unbolded "Release notes:" | 19:58 |
| beuno | the label/value there is unclear | 19:58 |
| bac` | for RN? | 19:58 |
| beuno | yes | 19:58 |
| bac` | you mean you want the bold back? | 19:58 |
| bac` | what do you think about the twisty hanging out to the side of changlog? | 19:59 |
| beuno | well, it seems to be the standard everywhere else, so yes for the bold | 19:59 |
| bac` | ok, <b>RN and CL | 19:59 |
| beuno | so, I like the idea | 20:00 |
| bac` | it used to say "There are no release notes" but i just got rid of the section if they aren't there | 20:00 |
| beuno | that's great | 20:00 |
| bac` | cool | 20:00 |
| beuno | the changelog, we don't show any of it, right? | 20:00 |
| bac` | not unless they twist | 20:00 |
| bac` | then it slithers in | 20:00 |
| bac` | and you're ok with the release date hanging out to the side of the h2? | 20:01 |
| beuno | ok, so it's not really "the full changelog" as much as the "the changelog" | 20:01 |
| beuno | I like what you did there in the h2, yes. I'd like to see it in staging with real data, but it's nice | 20:02 |
| bac` | beuno: oh, right. i c-n-p that part. didn't notice the text | 20:02 |
| bac` | beuno: great. i'll ask poolie later if this meets his concerns | 20:02 |
| === bac` is now known as bac | ||
| beuno | bac`, I'd just say "> Changelog" | 20:03 |
| bac | ok | 20:03 |
| beuno | I don't know about the date on the right though | 20:04 |
| beuno | seems too disconnected | 20:04 |
| beuno | and | 20:04 |
| beuno | infally | 20:04 |
| beuno | the "add download for release" is odd | 20:04 |
| beuno | why not just below each release? | 20:04 |
| bac | ok a '(+) Add download file' to the right below the table | 20:05 |
| beuno | yes, right-aligned | 20:05 |
| beuno | and you win | 20:05 |
| bac | great! thanks for the suggestions | 20:06 |
| === danilos is now known as danilo-afk | ||
| bac | beuno: what would you think about putting each release into a portlet to give them more separation from one another? | 20:12 |
| beuno | bac, it sounds good. Can you mock it up cheaply so we can look at it? | 20:13 |
| bac | done. let me snap it and scp | 20:13 |
| bac | http://people.canonical.com/~bac/download2.png | 20:14 |
| * beuno waits | 20:14 | |
| bac | this does not have the first one marked as 'top-portlet' as that will require some reorganization of the template | 20:15 |
| === sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| sinzui | EdwinGrubbs: do you have time to review https://code.launchpad.net/~sinzui/launchpad/package-link-validation-2/+merge/13727 | 20:18 |
| EdwinGrubbs | sinzui: sure | 20:18 |
| EdwinGrubbs | sinzui: I don't understand what it means that "the distroseries should also be linked". Is that a link on the page, or should the distroseries be linked with the productseries in the db? | 20:37 |
| EdwinGrubbs | sinzui: I get an error when I try to link "mozilla-firefox" on this page https://launchpad.dev/firefox/trunk/+ubuntupkg | 20:54 |
| EdwinGrubbs | sinzui: Even if the error is correct, the message should be on the field instead of having two errors above the form. | 20:55 |
| sinzui | ha | 21:03 |
| sinzui | EdwinGrubbs: I bet the example is bad sample data | 21:03 |
| sinzui | EdwinGrubbs: on edge +ubuntupkg has a table listing the distroseries and sourcepackage, but they are not linked. You need to hack the URL to see the object that the series is linked too | 21:04 |
| sinzui | EdwinGrubbs: https://launchpad.dev/firefox/+packages | 21:06 |
| sinzui | ^ See, that is why this view is broken, you just tried to create a duplicate link. Another series (0.9) is already the source pacakge | 21:06 |
| sinzui | EdwinGrubbs: did you notice that the error message had a link to the package: https://launchpad.dev/ubuntu/hoary/+source/mozilla-firefox | 21:08 |
| sinzui | ^ That shows that 1.0 is in fact the upstream for mozilla-firefox | 21:08 |
| sinzui | EdwinGrubbs: So for trunk cannot be the upstream for warty or hoary because those SPs are already linked. You can make grumpy Frozen there by making it the currentseries, then you can try to link trunk to mozilla-firefox | 21:10 |
| EdwinGrubbs | sinzui: It still seems like PackagingAddView.validate() should use setFieldError() instead addError(), or the separate "There is 1 error" message shouldn't appear. | 21:17 |
| sinzui | EdwinGrubbs: I think you are right | 21:18 |
| sinzui | EdwinGrubbs: my last addition to the base class does use setFieldError. The original code did not. I can quickly fix the whole class | 21:20 |
| sinzui | EdwinGrubbs: oh I see | 21:21 |
| sinzui | EdwinGrubbs: This is a bit tricky | 21:21 |
| sinzui | EdwinGrubbs: In the base class we do not know if there error is the series or the sourcepackage. In the new class, we know for certain that the error is sourcepackage. | 21:22 |
| sinzui | EdwinGrubbs: I could verify if the default_distroseries is not None and choose setFieldError() | 21:23 |
| EdwinGrubbs | ok | 21:25 |
| EdwinGrubbs | sinzui: It also seems strange to me to call packaging_util.packagingEntryExists(), when a single call to Packaging.selectOneBy() would allow you to make both checks, and it would allow you to say which productseries is already linked to the source package. | 21:27 |
| sinzui | It is not | 21:27 |
| sinzui | There are two issues and one is easier to fix than the oser | 21:27 |
| sinzui | other | 21:27 |
| EdwinGrubbs | sinzui: can there be multiple packaging entries for the same sourcepackagename and distroseries? | 21:28 |
| sinzui | The specific match probably means the *you* made a mistakes and you can fix it. The other error is someone else and you may want to investigate before making the chage | 21:29 |
| sinzui | No, but there are 81 in production | 21:29 |
| sinzui | Users trying to fix the data from the dsp, sp, p, or ps views are getting oopes as they struggle to fix the data | 21:30 |
| sinzui | EdwinGrubbs: a hack was put in place two years ago where we recognised this happens, but we only show the last one. this compounded the error because you cannot see the shadowed link to know it exists. the user is trying to add the link when he should be deleting the one he is seeing | 21:31 |
| sinzui | Except we never wrote code to delete a link to an SP | 21:31 |
| sinzui | There are a lot of oops that revolve around the the root cause that we let multiple links be created | 21:32 |
| sinzui | EdwinGrubbs: when this branch lands, it will not be possible to create a duplicate link, We can then fix the schema to not allow it to happen. My next branch is to allow the deletion on links to SPs | 21:33 |
| EdwinGrubbs | sinzui: is there a bug open to add a unique constraint on (distroseries, sourcepackagename) on the packaging table? | 21:34 |
| sinzui | Yes, it is on our list of bugs in progress that we review every morning | 21:34 |
| sinzui | https://bugs.edge.launchpad.net/bugs/196774 | 21:35 |
| mup | Bug #196774: It shouldn't be possible to link multiple productseries to a sourcepackage in a given distroseries <package-link> <Launchpad Registry:In Progress by sinzui> <https://launchpad.net/bugs/196774> | 21:35 |
| EdwinGrubbs | doh | 21:35 |
| EdwinGrubbs | sinzui: ok, that answers my questions. I'll send off the review in a sec. There are just minor style issues. | 21:35 |
| sinzui | okay. | 21:36 |
| === EdwinGrubbs is now known as Edwin-food | ||
| === salgado-sprint is now known as salgado-afk | ||
| sidnei | alright! | 22:16 |
| sidnei | Edwin-food: add https://code.edge.launchpad.net/~sidnei/lazr-js/yui-3.0.0/+merge/13737 to your queue when you get back | 22:16 |
| sidnei | uhm. i don't know how to get a proper diff i fear. | 22:18 |
| === matsubara is now known as matsubara-afk | ||
| === Edwin-food is now known as EdwinGrubbs | ||
| sidnei | EdwinGrubbs: ok, ignore that mp, take this one: https://code.edge.launchpad.net/~sidnei/lazr-js/yui-3.0.0-redux/+merge/13743 | 23:01 |
| EdwinGrubbs | sidnei: what happened? | 23:02 |
| sidnei | EdwinGrubbs: the order i merged the two base branches made it hard to generate a meaningful diff | 23:02 |
| sidnei | EdwinGrubbs: this one has a link to a paste with the right diff. | 23:02 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: sidnei || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| EdwinGrubbs | sidnei: how did you create the diff? I would like one that ignores whitespace changes, so I can focus on what really changed. | 23:12 |
| sidnei | EdwinGrubbs: bzr diff -rancestor:../jstestdriver-support | 23:13 |
| sidnei | EdwinGrubbs: and then prune the 10k lines of the new YUI :) | 23:13 |
| sidnei | and the yui-patch.js file, which is copied verbatim from upstream jstestdriver | 23:14 |
| sidnei | i guess i need --diff-options | 23:14 |
| EdwinGrubbs | ugh | 23:22 |
| sidnei | yeah, ugh :( | 23:25 |
| EdwinGrubbs | sidnei: don't worry about getting a new diff, I see why diff was showing me more lines than I thought it should. You moved some javascript from the top of the file to the bottom. I'll handle that change. | 23:34 |
| sidnei | ok, thanks! | 23:34 |
| sidnei | EdwinGrubbs: i need to go to a funeral, but will be back in a bit. have fun! | 23:39 |
| EdwinGrubbs | sidnei: I'm sorry to hear that. | 23:39 |
| sidnei | EdwinGrubbs: yeah. not a close person, but it sucks anyway. im happy as long as isn't mine *wink* | 23:40 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
