=== jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === gmb` is now known as gmb [09:20] jtv: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-507642/+merge/20978 ? [09:20] adeuring: how dare you ask such a thing [09:20] especially of an oc... [09:20] oh, wait, I guess I can [09:21] well I like to be a bit bold ;) === jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: *adeuring* || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:24] adeuring: bold enough for you [09:24] ? [09:24] jtv: sure ;) [09:25] adeuring: bug snapshots are new to me... what is that? [09:26] jtv: There is a calss Snapsshot somewhere... let me search again where exactly... [09:26] adeuring: not Schnapps-shot, surely [09:27] jtv: erm, yes ;) [09:27] lazr.lifecycle..? [09:28] jtv: yes [09:29] jtv: anyway, for POSTs to the webservice API, a snapshot of the changed object is created. [09:29] jtv: is it OK if I enqueue https://code.edge.launchpad.net/~al-maisan/launchpad/partner-expiry-535030/+merge/20990 ? [09:30] and this involves creating shortlists of collection fields [09:30] al-maisan: okay; I'm not too well though so be prepared for me dropping out at some point [09:30] and if these collections are not short enough, the request fails. [09:30] adeuring: ok, I'll try to work with that [09:33] adeuring: do I understand this correctly? Snapshots are made of these lists on a bug, and that process runs into the hardlimit. So you stop snapshots from being made, and to test this, you set a tighter hardlimit for snapshotting and prove that you're not running into it? [09:33] jtv: exactly [09:34] adeuring: that sounds a bit like you're second-guessing doNotSnapshot [09:34] jtv: but the limit is reduced for tests only ;) [09:34] yes, I got that :-) [09:35] jtv: and yes, that's the core idea of the test. [09:35] jtv: it is a kind of simulation of the situation we have currently when you try to subscribe to bug 1 [09:36] Bug #1: Microsoft has a majority market share

adeuring: let's say I were to have a lot of faith in my fellow developers and reviewers in lazr-lifecycle. Then I'd sort of expect a very similar test to have happened in their code already. [09:36] yes thank you mup, we hadn't forgotten that one [09:37] jtv: well, perhaps. But what would this mean for the changes in my branch? [09:37] adeuring: that I think you're doing too much work [09:38] maybe... [09:38] adeuring: I haven't read the full diff yet, but how about just testing that these fields are marked as doNotSnapshot, and saying why they have to be? === al-maisan changed the topic of #launchpad-reviews to: on call: jtv || reviewing: *adeuring* || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:39] jtv: let's assume that the necessity arises that we must include one of the collections again in the snapshot. [09:39] adeuring: and frankly I'm much more interested in the functional consequences of not snapshotting these things... what differences will it make to users besides not exploding? [09:40] adeuring: that's a very good assumption; but if your test can just show _that_ the fields are doNotSnapshot, then your test will trip this up. [09:40] jtv: I haven't noticed any. Discussed this with other people in the bugs team -- and all we am eup with: "try possibly relevant tests" [09:40] heh [09:40] and none of those I tried did fail [09:42] jtv: well, if we simply check a doNotSnapshot "decoration", we do not check if POSTs succeed or fail [09:42] stub, gmb: thanks for reviewing my branch to change the update-pkg-cache dbuser, however the diff that LP generated is total garbage! [09:42] bigjools: Really? [09:42] I think a db-devel/devel mismatch [09:42] bigjools: branched off devel, mp'ed for db-devel? [09:42] bigjools: Hah. [09:43] Oh, jow. [09:43] jtv: other way around [09:43] Joy, even. [09:43] bigjools: then you're an idiot [09:43] :-P [09:43] jtv: naturellement [09:44] not sure how I managed it since I targeted the MP quite deliberately :/ [09:44] bigjools: Well, send [09:44] adeuring: then again, you're dealing with a very specific failure in snapshotting, not one with posts per se [09:44] er [09:44] bigjools: ... me the actual diff and I'll take a look in a little bit. [09:44] adeuring: support for POSTs in general should be integration-tested, but I assume it is. [09:45] jtv: no you were right t he first time [09:45] gmb: http://pastebin.ubuntu.com/392395/ [09:45] jtv: fine, but I don't get your point... [09:46] Hah. [09:46] bigjools: Yeah, I think you can land that :) [09:46] adeuring: we know that POSTs normally succeed. You're dealing with one very specific failure in that process. I'm saying you could test for the fix to that specific failure, rather than test the full integration chain. [09:46] jtv: OK, so, what sort of test would you suggest? [09:47] just that these properties are not included in the snapshot? [09:47] gmb: ta :) [09:47] I think that's not a good idea [09:47] adeuring: a check that the necessary lists are doNotSnapshot, with a comment saying why this is needed so that people don't mindlessly change the test when it breaks. [09:47] jtv: well, ok... [09:48] adeuring: separately, it could be nice to have the snapshotting mechanism improved for this sort of situation: don't snapshot if the lists get too long, or something. [09:48] jtv: perhaps [09:48] The shortlist is presumably there as a way of detecting that the mechanism doesn't scale up to the things it's being used for, and that this sort of change may be needed. [09:49] adeuring: if/when the mechanism changes, you'd probably find that you lost your ability to reproduce the failure, and there'd be no test failure to indicate this. === allenap changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: *adeuring*, - || queue: [al-maisan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:49] Morning jtv :) [09:49] jtv: good point [09:49] hi allenap! [09:52] jtv: triggering the failure in the test is easy. [09:52] jtv: ah, no... [09:53] adeuring: it would also be nice AFAICS if you could test for a realistic limit, so e.g. adding 1K subscribers; then once the snapshots are fixed, you could perhaps remove the doNotSnapshot and still have a test that proved that you didn't break things. But you don't need to repeat that for all those fields AFAICS [09:54] (And if you're going to create persons in the factory, be sure to use makePersonNoCommit :) [09:54] jtv: hrmm... I thoughy you suggested to only test for the "doNotSnapshot" decoration? [09:54] s/thoughy/tought/ [09:55] adeuring: I'm trying to meet you halfway here. :) [09:57] adeuring: actually, I'm just agreeing with you that it does make sense to try and show that the posts will now succeed. But I'm also saying it makes sense to keep two parts of that separate: (1) doNotSnapshot fixes the problem, and (2) doNotSnapshot is applied in all the necessary places. [09:58] That way the testing problem goes from (test for cause of problem) × (test places where problem can happen) to (test for cause of problem) + (test places where problem can happen) [09:59] And if (test for cause of problem) involves a hard-limit of 1,000 objects.... :-) [09:59] jtv: OK. But I not sure if creating 1000 persons without a commit works here. Doesn't the logout() calls imply a commit? [10:00] adeuring: I think it does, yes, but that's just once—I'm saying avoid committing a thousand times inside the loop. Also, don't do this in the setUp since only one test will be needing it. If you're lucky you can get away with these persons being flushed to the db but never committed. [10:01] jtv: right [10:01] factory.makePerson secretly commits [10:01] ...after calling factory.makePersonNoCommit. [10:01] The commits are usually only needed afaik if you're planning to log in as the new person. [10:26] al-maisan: before I (or allenap) gets around to an actual review... you're changing and adding some %s to big queries. Maybe it's getting to the point where it's better use a dict there instead. "%(x)s + %(y)s = %(z)s" % {'x': 1, 'y': 2, 'z': 3} [10:27] jtv: good point .. can do that. [10:27] Helps avoid annoying breakage while editing :) [10:27] yup === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: (adeuring, al-maisan), - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [10:36] allenap: couldn't help myself, got drawn into muharem's branch :) [10:38] al-maisan: s/\/but referring/g [10:38] jtv, thanks, will correct that. [10:38] al-maisan: but is it really useful to enumerate what you're about to test? [10:39] jtv: That's okay :) I'm still getting my stuff together here. [10:41] jtv: I pushed a new version of my branch to LP [10:41] al-maisan: I think it'd be more useful to explain for each of the test classes that you derive from the common-tests class what is specific to them. [10:41] adeuring: kuhl, danks [10:42] jtv: I see .. let me do that then :) [10:43] al-maisan: also, I don't know how runScript goes about its job, but it may be worth checking that it's not forking off a subprocess each time. Otherwise you spend an eternity re-reading ZCML, and recovering the db afterwards. [10:43] jtv: I'll have a look. [10:44] thanks [10:46] al-maisan: I see you're running into complications loading the tests. I think you can avoid those by not deriving ArchiveExpiryCommonTests from ArchiveExpiryTestBase; instead the specific test classes can multiply-inherit from both of those two. That way the test loader doesn't come under the impression that ArchiveExpiryCommonTests is a test case. [10:47] adeuring: oooh, so much tighter! [10:47] jtv: will try that as well, thanks :) [10:48] adeuring: in test_no_snapshots_for_large_collections, can you add a note saying why there should be no snapshots (i.e. that the lists can get large enough to break the snapshot)? [10:49] I like the bug references in the test BTW. [10:49] jtv: I can do that, but I thinks the the doc sting of the test class already has this note [10:50] adeuring: ah yes... that's cramming a lot of information into one sentence though. [10:50] yeah, I have some bad habits ;) [10:50] (English is a relatively impatient language :) [10:51] adeuring: an American once wondered out loud what his country would have looked like if German had become the official language (as it almost did). I told him people would be more polite in conversation, since you won't know what somebody's really saying until they've finished the last verb at the very end of the sentence. :) === matsubara-afk is now known as matsubara [10:52] jtv: could that be Mark Twain? He wrote some really hilarious comments about German [10:53] adeuring: unfortunately I have never been in the position to make smart-alec remarks to Mark Twain. The scathing comebacks could have provided some jolly good fun at my expense. [10:53] This was an American who is alive today. Or at least, was yesterday. [10:54] adeuring: "salgado-change-anything"..? [10:54] jtv: ??? [10:54] adeuring: hey, it's from _your_ test :) === noodles785 is now known as noodles775 [10:55] jtv: Ah, right... That's simple copy&pasted... [10:55] jtv: some modern comments about German and Germany: http://www.amazon.de/die-was-Ein-Amerikaner-Sprachlabyrinth/dp/3499622505/ref=sr_1_1?ie=UTF8&s=books&qid=1268218456&sr=8-1 A really nice read [10:55] adeuring: ah. :) Also in that test, could you turn the range(5) into range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1)? [10:55] jtv: sure [10:56] Labyrinth is feminine in German? [10:56] ah no, evidently not [10:56] The URL suggests that, but the page title makes clear that it isn't. [10:58] "Das Amerikan in das sprach labyrinth" [10:59] yes. Really odd stories. He noticed a sign "durchgehend geöffnet" at a aupermarket and assumed he could buy stuff in the night... [10:59] Frankly I wouldn't have known better... [10:59] jtv: sure, [11:00] that requires strange "context knowledge" [11:00] "durchgehend" means "not closed during lunch" ;) [11:00] ah! [11:01] adeuring: I think your branch is nearly there... the setting and resetting of the snapshot limit is a bit ugly. How about a "with"? [11:02] Not particularly urgent, but practicing it now may pay off in the future. :) [11:02] jtv: yes, I thought about that. But this fidding with a constant is done just here, so I did not see the point to use "iwth" [11:03] adeuring: sure, no worries. If you're updating the range(5), r=me [11:03] jtv: thanks! [11:03] I'll go do the paperwork. === jtv changed the topic of #launchpad-reviews to: on call: jtv, allenap || reviewing: al-maisan, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [11:10] jtv: BTW runScript() does not for off sub-processes.. [11:10] al-maisan: good to know, thanks for checking it out. [11:27] jtv: the enhancements you suggested: http://pastebin.ubuntu.com/392443/ [11:28] al-maisan: looking... [11:45] al-maisan: sorry, got sidetracked for a moment (figuring out why my job wasn't selected as a candidate). Looking at your pastebin, too much indentation (or use of tabs) in param_names & param_values [11:46] jtv: right, will fix the indentation. [11:46] al-maisan: why not just create a dict directly? Less fragile as the code continues to evolve [11:46] jtv: fine. [11:47] al-maisan: I'm not sure if sqlvalues will work on a dict, but it would be nice if it did. [11:47] al-maisan: but apart from that, I approve of the branch. [11:47] jtv: I don't think so .. that's why I used the dict(zip()) approach [11:47] al-maisan: you may want to use quote() on a single value instead of sqlvalues() on a bunch [11:49] jtv: hmmm .. that works for simple params .. in the code in question I have query params that are tuples ('archive_types') .. would quote work for them as well? [11:49] al-maisan: I think you need sqlvalues for those [11:50] jtv: in which case I am stuck with dict(zip()) .. [11:51] al-maisan: or just doing { 'key': quote(value), ...} [11:51] (Which doesn't look all _that_ bad to me) [11:52] jtv: IIRC there's also an sqlvalue() function .. let me check. [11:52] al-maisan: sqlvalues will also work on one value though [11:53] jtv: ok .. I can use it like that then. [11:54] al-maisan: another question... where in the code was the candidate selection query again? === jtv 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 [11:55] allenap: I'm bowing out in a minute [11:55] jtv: let me look [11:55] jtv: Okay, have a good day :) [11:55] thanks, you too [11:56] jtv: lib/lp/buildmaster/model/builder.py, line 475 [11:57] .. and the lines preceding it [11:57] al-maisan: thanks... it's not selecting my arch-independent job because the Builder's virtualized seems to be set to False. [12:03] jtv: actually sqlvalues() does support named parameters and returns a dict in that case. [12:03] that's smart [12:06] jtv: so, here we go: http://pastebin.ubuntu.com/392467/ === mrevell is now known as mrevell-lunch [12:10] al-maisan: sheer beauty. I've already approved the MP. [12:14] jtv: thanks :) [12:15] np [12:16] * jtv rushes off [12:36] danilos, henninge hi, can we schedule a preimplementation chat for bug 201749? [12:36] Bug #201749: Impossible to mark as needing review strings already translated or with suggestions [13:13] adiroiban, sure, we can do it now if you want === henninge_ is now known as henninge [13:17] ok. now, if you submit an empty translation, it will add a new translation and link it to the msgset (for a language) [13:17] is this correct? === matsubara is now known as matsubara-afk [13:36] and I was thinking that in the translationmessage, insead of adding a reference to an empty string, it should change the "is_current" to false [13:36] so that all msgstr linked with the current msgset are false === mrevell-lunch is now known as mrevell === jamalta-afk is now known as jamalta === salgado changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:25] allenap, care to review https://code.launchpad.net/~salgado/launchpad/edge-redirect-bugs/+merge/21054 for me? [14:26] salgado: Sure! [15:01] allenap: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-528569-api-bug-search-for-linked-branches/+merge/21058 ? === matsubara-afk is now known as matsubara === allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: salgado || queue: [adeuring] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:10] adeuring: Sure! [15:11] allenap: thanks! === jamalta is now known as jamalta-afk === jamalta-afk is now known as jamalta [15:28] allenap, can I jump on your queue? [15:29] rockstar: Go for it :) === rockstar changed the topic of #launchpad-reviews to: on call: allenap || reviewing: salgado || queue: [adeuring, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:29] allenap, https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025 === jamalta is now known as jamalta-afk === salgado is now known as salgado-lunch [16:23] allenap, can I add another branch to your queue? [16:27] EdwinGrubbs: https://code.edge.launchpad.net/~sinzui/launchpad/api-bug-tracker-0/+merge/21061 [16:29] adiroiban, hi [16:29] ok [16:29] I have also added a check in the browser layer [16:29] to check for „forged” needs_review or diverge form fields [16:29] adiroiban, so, what bug was that? (it seems you named the wrong one on #u-t [16:30] bug 201749 [16:30] Bug #201749: Impossible to mark as needing review strings already translated or with suggestions [16:30] adiroiban, or maybe not, not sure why it said it was private on #u-t [16:30] ... well.... a bug? [16:31] rockstar: A combination of a mammoth pre-implementation call with gmb, a car in need of repair, two cinema tickets and some pesky kids means I almost certainly won't get to your review today. I am very sorry, but I am happy to do it first thing in the morning if that's of interest to you. [16:32] allenap, I can find someone else. Thanks. [16:32] adiroiban, yeah, it was ubotu there, and it's mup here, ubotu might be broken :) [16:33] allenap, I know how the EOD reviewer day goes. :) [16:33] rockstar: Since since it was my fault that allenap was away for a while, I'll take your review. [16:34] adiroiban, first off, all comments should be full sentences that end with punctuation, so don't remove a period in one of those: and doing that when discussing things makes the diff larger and harder to read :) [16:34] rockstar: https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025 ? [16:34] gmb, aye. Thanks a lot. [16:35] adiroiban, ok, so what you are trying to do will cause a lot of trouble [16:35] adiroiban, it won't necessarily have the effect you want it to have [16:36] adiroiban, for instance, if current translation is diverged, and there is a shared one, this will make the shared one current, and the diverged one might show as a suggestion only accidentally (i.e. if it has a date_created newer than the shared one) [16:36] when I tick the „needs review” box, I do it because I think that the current translation is wrong [16:36] rockstar: Looks good. r=me. [16:37] gmb, ta [16:37] adiroiban, right, but there is also that 'needs review' side of things that might not really work as desired [16:38] adiroiban, I agree unsetting it as current is an improvement over existing behaviour, but you can't do that as simply as unsetting a flag either; if it's diverged and there's an existing identical suggestion, one of those needs to be removed [16:39] adiroiban, finding all that out is pretty expensive, so I think a proper solution would be to introduce a new column on translationmessage table called something like is_reviewed [16:39] danilos: OK. Right now, I have no idea how diverged translations should be handled [16:40] adiroiban, it's all very, very complex, and there are some intricate bugs as well; doing it this way would just introduce more intricate bugs [16:41] adiroiban, in short, there's no simple solution to this [16:41] gmb, do you think you could do another quick one? Mostly just mechanical changes for javascript adhere to the style guide. [16:42] rockstar: Sure. [16:42] adiroiban, the solution would probably be to create a 'unsetTranslationMessage' on POTMsgSet that takes a template, language, variant as parameters [16:42] danilos: I'm trying to construct a test scenario for the use case described by you above [16:42] adiroiban, and then we can call that when we figure out that's what a user wants, and it'd be isolated from the rest of the updateTranslation (which is a mess, and should not be extended in any way) [16:44] adiroiban, look at lib/lp/translations/tests/test_potmsgset.py test_updateTranslation tests and how complex they are [16:45] adiroiban, making them even more complex is out of the question :) separate method which encapsulates the concrete meaning of the action is the way to go if you want to tackle this [16:45] ok [16:45] danilos: so I will move this logic in a new method [16:46] adiroiban, then you can worry about all the peculiarities of sharing and divergence for your specific use case in that particular method, because they'll be different [16:47] danilos: unfortunately, I don't know what should I do for divergent and sharing messages [16:47] adiroiban, right, and then you will have to come up with a good story for divergence and sharing; the biggest complication with all that is that diverged messages might be duplicated so once they are not used anymore, they should be aggregated [16:47] but if those cases have tests [16:47] I guess I can run the tests [16:47] adiroiban, well, for one, once you unset is_current, you should unset TM.potemplate as well [16:47] and improve the implementation [16:48] adiroiban, yeah, I'd be happy to help explain as much as possible [16:48] adiroiban, there are a lot of tests, and they are still not complete [16:48] danilos: translationmessage.potemplate ? [16:48] looking the in DB [16:49] that field is not set for all translations [16:49] adiroiban, yes, that indicates that that message is a divergence for a particular template (i.e. one coming from jaunty, where the shared version is used in karmic, lucid,...) [16:49] adiroiban, if potemplate IS NULL, you are looking either at a shared current/imported message, or at a suggestion [16:50] I see [16:50] ok [16:50] alse [16:50] else [16:50] is the ID of the specific potemplate [16:50] adiroiban, ideally, that would have been pofile, and we'd lose (potemplate, language, variant) on TM, but since we did incremental development, it was impossible to re-use pofile [16:50] adiroiban, that's right [16:51] ok [16:51] adiroiban, TM.pofile is basically an old field that's not up-to-date or correct anymore [16:51] I will take care of the TM.potemplate [16:51] ony other exception [16:51] ? [16:51] any other exceptions? :) [16:51] adiroiban, well, a bunch of corner cases that will pop up, I am sure [16:52] adiroiban, so, I'd have to ask you to get that branch past a Translations reviewer as well (nobody else would be intimate enough with the code), and we'll have to talk some more [16:52] adiroiban, I'd also have to carefully think about all of the exceptions and problems [16:53] adiroiban, basically, a TM is unreviewed suggestion if it has a date_created newer than the current translation; it means that the simplistic approach will just show all old (even those reviewed long ago) suggestions, which would be bad [16:54] adiroiban, because unsetting a currrent translation will remove the date_reviewed info, and then all suggestions would end up being unreviewed [16:54] adiroiban, so, that's another case to worry about; and when you introduce message sharing/divergence into it, it gets even "nicer" :) [16:58] danilos: why is bad if you see all suggetions ? [16:59] adiroiban, because some have already been reviewed [16:59] adiroiban, and rejected [16:59] danilos: yes [16:59] but some were rejected by mistake [16:59] adiroiban, that's a different bug ;) [16:59] or were rejected based on some translation rules [16:59] that was now changed [16:59] adiroiban, well, different problem [17:00] adiroiban, we don't want them all to show up on a regular +translate page [17:00] danilos: but we can not tell which suggestion was already reviewed [17:00] adiroiban, yes we can [17:00] adiroiban, as I said, there's currently a clear concept of what a reviewed message is [17:00] adiroiban, anyway, I've got a call now, ttyl [17:00] ok [17:00] I will move the code in a separate logic [17:01] and will consider the cases we discussed here [17:01] and then we can have a new review [17:02] rockstar: Sorry... is it https://code.edge.launchpad.net/~rockstar/launchpad/code-js-reorg/+merge/20170 you want me to look at? [17:03] gmb, yes [17:03] rockstar: Cool. Looking now. === allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: adeuring || queue: [rockstar] || 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: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [17:13] rockstar: Looks good. r=me. === salgado-lunch is now known as salgado === allenap changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [17:33] gmb, so, the first branch that you reviewed - abentley_ and I just talked about it, and he suggested using a decorator to do the try/catch instead of doing it inside the method. [17:33] (because what I was doing was crackful) === abentley_ is now known as abentley [18:03] adiroiban, I am about to call it a day, is there anything I can do to help out while I am still here? :) [18:03] yes [18:03] can you please explain [18:04] in simple words [18:04] what should „needs review” do [18:04] from the point of view of users [18:04] it should just set the translation to empty [18:04] and mark the current translation as needs reviewn ? [18:04] review? [18:05] danilos: ^ :) [18:05] or it should dismiss the current translation? [18:06] and just leave the message untranslated [18:06] adiroiban, well [18:06] adiroiban, I think it should do that and leave all the other unreviewed suggestions as unreviewed suggestions [18:07] adiroiban, the message should end up untranslated, but with the previous active translation as another suggestion added to the existing set of suggestions [18:07] ok [18:08] and then, listing ALL suggestions should be another bug and should be displayed only in the zoom in view [18:08] adiroiban, btw, look at dismissAllSuggestions for a few ideas [18:08] adiroiban, that's right, and there's already that "another" bug filed :) [18:09] danilos: yes. I think I am assigned to that bug [18:09] adiroiban, bug 339507 [18:09] Bug #339507: translation page should show old translations [18:09] adiroiban, right [18:10] but I was thinking we can force showing all suggestions [18:10] adiroiban, and do take care of the TranslationConflict in a similar way (when someone changes translations after you have loaded the page) [18:10] by ticking the needs review [18:10] adiroiban, we shouldn't [18:10] danilos: ok :) [18:10] adiroiban, we'll get a very confusing behaviour of 'needs review' [18:11] danilos: ok. Many thanks! [18:11] adiroiban, now, you can perhaps say "it would make the implementation much easier, so I'd do that as a first step/branch", and then I'd agree :) [18:11] allenap, did you see my reply to that review you did for me? [18:11] adiroiban, you are welcome [18:11] danilos: :) [18:11] I will try to see how I will manage the implementation [18:12] and if I needs to split this job [18:12] adiroiban, sure, email whenever or we can talk some more tomorrow :) cheers [18:12] by [18:12] have a nice evening [18:12] adiroiban, thanks, so do you === gary_poster is now known as gary-lunch [18:59] abentley, check out this (much better) diff. We get the real exception now. :) [19:00] https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025 [19:30] gary, do you hvae a minute to review https://code.edge.launchpad.net/~leonardr/lazr.restful/version-descriptions/+merge/21075 ? it shouldn't take long === gary-lunch is now known as gary_poster [19:42] leonardr: yes [19:46] Where have all our OCRs been the last two days? [19:46] leonardr: "[x for x in contents['service_doc']]" -> "list(contents['service_doc']]" ? [19:47] leonardr: lines 189 and 191 of diff [19:47] gary: sure [19:47] cool [19:48] leonardr: do you intend to keep pdb around? line 286 [19:48] gary, no, that's in by mistake [19:48] cool [19:49] leonardr: otherwise, r=gary. Will mark as such. [19:49] gary: also the mssing = object() needs to go [19:49] ah, ok. Should have investigated more carefully then. [20:09] * kfogel is away: bbiab === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk [21:29] mwhudson, could you do a small review for me, mostly mechanical changes? [21:30] (it's javascript) [21:31] rockstar: i can try! [21:42] mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/code-js-file-reorg/+merge/21085 [21:43] mwhudson, sorry it's so boring. :( === jamalta-afk is now known as jamalta [22:13] mwhudson, as a matter of fact, the next branch is the branch that moves the javascript into lib/lp/code/javascript [22:13] rockstar: :) [22:19] https://code.edge.launchpad.net/~thumper/launchpad/new-import-link-move/+merge/21088 for someone [22:19] diff should be along shortly [22:21] diff is up [22:21] thumper, looking now. [22:22] rockstar: ta [22:22] thumper, r=rockstar [22:22] * rockstar runs all the Windmill tests again. === jamalta is now known as jamalta-afk