[00:02] StevenK: It depends how you use setUpFields() to fiddle with the schema. [00:29] wgrant: http://pastebin.ubuntu.com/1013974/ [00:30] StevenK: Myes? [00:38] wgrant: I was having breakfast -- that diff is in relation to the question I asked you earlier. [00:40] StevenK: If it works, great. If it doesn't work, that doesn't tell me why it doesn't work :) [00:42] cjwatson: have you not been exposed to my 'imperative bug reports considered harmful' meme ? [00:43] I don't think I used imperative "should" anywhere in that [00:43] Oh, the title. Whatever. Rephrase as you see fit; I must say I entirely fail to see the point in that meme. [00:43] cjwatson: the title is imperative;) - its an imperative I agree with :) [00:43] And I generally have trouble figuring out what you think I'm supposed to write instad. [00:43] *instead [00:45] in this case, I'd have said something like 'queue tool requires direct DB access' [00:45] OK. I don't really see the benefit in spending time on consolidating the grammatical voice of bug titles, though; it seems like a very minor point at best. [00:47] Perhaps this is the natural result of developers needing to file bugs for work they already have in progress in order to provide a place for QA tags to go. [00:47] wgrant: http://pastebin.ubuntu.com/1013992/ [00:48] That'll tend to result in people describing what they're doing rather than describing the original problem. [00:48] Anyway, rephrased the title, but I can't promise to remember in future. [00:49] cjwatson: I think the QA story is indeed part of the situation [00:50] cjwatson: I find imperative bugs from devs or non-devs are equally likely to age terribly; yours wouldn't because the description is comprehensive [00:50] StevenK: Sounds like you have information_type in field_names. [00:51] cjwatson: also you're about to land a change, which makes the aging aspect less of a concern [00:51] wgrant: I'm supposed to? [00:52] StevenK: The default LaunchpadForm.setUpFields sets up fields that you specify. [00:53] lifeless: Well, hoping that I actually get all the way through the several incremental changes in this series ;-) [00:53] StevenK: Unless information_type is in the schema that setUpFields is using, putting it in field_names isn't a very good idea. [00:53] StevenK: The fact that you override setUpFields to add in information_type manually suggests that it doesn't belong in field_names. [00:56] wgrant: If I drop it from field_names then it doesn't show up at all [00:58] StevenK: Ah, perhaps the default form template uses field_names to determine the order. [00:58] StevenK: Why did you move information_type_field into setUpFields anyway? [00:58] It doesn't depend on anything there, [00:59] StevenK: Normally you'd tweak field_names at some point, not just overriding the schema field because you can. [00:59] wgrant: I need to override the schema field for the vocab [00:59] Also, super() [01:00] This isn't Python 2.1. [01:02] Same error with super() [01:02] super() is unrelated here, it's just that your code was looking like it belonged in 2002. [01:04] StevenK: Grep for form_fields.omit [01:04] To see the usual pattern. [01:05] wgrant: I can put it in BranchEditSchema, but the vocab doesn't respect the flags [01:06] StevenK: If you really can't put it in the schema, you'll probably have to use the super/omit/+= approach. [01:07] wgrant: Right, so it stays in field_names, but then I omit and re-add it [01:07] StevenK: Except that this whole thing wants to be feature-flagged. [01:08] Observe BugSecrecyEditView [01:19] lifeless: I don't mean to be awkward, by the way. Just found it hard to see the importance of it. If you think it makes a measurable difference to how useful bugs are a while after they've been filed, fair enough. [01:20] cjwatson: The prototypical poor bug looks like this [01:20] "Do X" [01:20] "Something detailed about the change" [01:21] usually, when I ask the filer 6 months or a year later, they have no detailed recollection for what prompted them to file the bug [01:21] that is, the symptoms are long forgotten [01:21] Yeah, I can see that being troublesome [01:22] so my actual thing is that we should always record the symptoms [01:22] and that as a mind-hack, avoiding imperatives seems to encourage capturing the symptoms vs the fix [01:22] Ah, see, I would have found "record the symptoms" a much clearer way to put it than "no imperatives". [01:22] Your mind-hack bounced off me :) [01:23] indeed [01:24] the other, *much weaker* aspect is that in bug listings, having symptoms visible helps figure out if you're looking at the right bug [01:24] (Because, I think, "no imperatives" reminded me too much of pseudogrammatical nonsense like "passive voice is naughty".) [01:24] I say weaker because you can click through and if the description is maintained or just good, its only time [01:24] cjwatson: yes, martin pool also fell down that path [01:25] I need to evolve how I present this, I think. [01:25] hello, i'm michael i work for a software company called 'pedantical' [01:25] MPT has filed dozens, if not hundreds of these poor bugs btw [01:25] with pages of exposition [01:25] but missing the actual symptoms. [01:25] He changed some years in and started recording symptoms too [01:26] its quite dramatic when you scan through a couple K bugs, what patterns the brain finds :) [01:26] wgrant: Right, so I was attempting to do the same, but there's an adapter in BranchEditView [01:26] StevenK: An adapter? [01:27] def adapters(self): [01:27] """See `LaunchpadFormView`""" [01:27] return {BranchEditSchema: self.context} [01:27] With a @property [01:28] StevenK: Um [01:28] StevenK: BranchEditView already has a tonne of logic for this sort of thing. [01:28] Sometimes showing the private field, sometimes not [01:28] Using custom widgets [01:28] in setUpFields. [01:28] wgrant: Yes, have you seen my diff? [01:29] StevenK: Yes, and it doesn't make much sense. [01:29] Because it's defining a setUpFields when there already is one. [01:29] Unless you're defining it on a superclass -- hard to tell from that diff. [01:30] wgrant: Sorry, I meant the total diff. http://pastebin.ubuntu.com/1014035/ [01:30] Ah, BranchEditFormView [01:30] Yes. [01:30] * StevenK stabs it [01:30] StevenK: Line 258 [01:31] Ah, so the setupFields in BranchEditFormView will never get called [01:31] Correct. [01:31] THis is why we shouldn't write code like it's 2002. [01:32] Because it's not 2002. [01:32] wgrant: But the adapter I pasted above is why I can't do what BugEditSecrecyView does [01:32] In fact, maybe I should grep for anything calling a method on a CamelCase identifier and kill it :) [01:34] StevenK: Why can't you put it in BranchEditSchema? Does the vocab evaluate the flag at instantiation time? [01:38] wgrant: It was there originally, but it looks like the flag doesn't take effect [01:39] I'd perhaps work out why it's not taking effect. [01:42] wgrant: The class is at the top level, and the vocab has a () at the end so it happens at file read time? [01:43] StevenK: If you instantiate it there, it will be insantiated there, indeed. [01:43] wgrant: So I was trying to work around that like BugEditSecrecyView but that adapters thing uses the schema [01:44] StevenK: Are you sure you can't override it? [01:45] I mean, you may be able to do the usual thing in setUpFields. [01:45] Or define the interface in a method somewhere. [01:45] Or not instantiate the vocab at that point (I'm not sure if you have to) [01:45] Or not use adapters. [01:45] etc [01:45] one of those ways will work [01:46] wgrant: Why is the adapter even there? [01:47] StevenK: Probably to adapt the data into the form required by the interface. [01:47] Now [01:47] I'm not sure it's entirely useful here, given that the adapter does nothing. [01:48] It gives a fail to adapt if I remove it [01:50] Well, yes, clearly removing the adapter is going to not work very well. [01:51] wgrant: But you said it does nothing! :-) [01:51] The adapter internally does nothing. [01:51] Its presence does something, but it's probably not useful. [01:51] That doesn't mean you can just delete its definition without consequence. [01:51] If I change it from using BranchEditSchema to self.schema it also gives fail to adapt [01:52] That's not really surprising, since self.schema is BranchEditSchema. [01:52] So [01:52] What I would try is the usual approach. [01:52] But they should be equal :-( [01:52] Define the class at view instantiation time. [01:53] THey aren't just equal -- they are the same object. [01:53] So changing from one to the other isn't going to change anything -- object identity holds between them. [01:53] wgrant: I was doing that with def schema: class schema... [01:53] StevenK: Right, that's probably the least invasive approach here. [01:53] Try something that works. [01:54] If it works, it's possibly a good idea. [01:54] If it doesn't work, it's probably a good idea to try something else :) [01:57] TypeError: ('Could not adapt', , ) [01:58] StevenK: You're probably creating multiple instances of the class. [01:58] Erm, wait, that's not a class defined inside the class [01:58] You're still using the module-level one. [01:59] wgrant: http://pastebin.ubuntu.com/1014057/ [02:00] StevenK: That has the obvious problem that the interface will be recreated each time self.schema is evaluated. [02:01] Which is less than ideal, as the that means self.adapters will be returning a new interface each time, so it's entirely useless. [02:01] I thought that was BugEditSecrecyView was doing? [02:01] It's not adapting. [02:02] Ah, true. [02:02] It probably only evaluates self.schema once, and even if it does it multiple times it doesn't care about their identity. [02:02] Just their content. [02:02] Adaption cares about identity. [02:03] :-( [02:03] hmm [02:03] anyhone happen to know, will the PPR consider e.g. memcache time as 'SQL' time ? [02:03] lifeless: I believe so. [02:03] wgrant: +login [02:03] lifeless: AFAIK its logic predates non-SQL timedactions [02:04] 1.4s 99th percentil, 0.2s 99th percentil of SQL [02:04] so 1.2 seconds doing [02:04] Hm, maybe it's smarter than I thought [02:04] We track at least one of our calls to SSO [02:04] possibly the nested calls thing is it [02:04] ah yes, that will be showing 0ms times [02:04] Launchpad Production Errors [02:05] OOPS Tools Production Errors [02:05] Production Errors [02:05] FAIL [02:05] sql_statements = da.get_request_statements() [02:05] sql_milliseconds = sum( [02:05] endtime - starttime [02:05] for starttime, endtime, id, statement, tb in sql_statements) [02:05] StevenK: if thats mail, great success [02:05] lifeless: Oh, prod OOPSes are going to oops.c.c now? [02:06] yes [02:06] easing over [02:06] lifeless: Oh, excellent. So now developers have to skip six useless mails a day rather than four [02:06] :/ [02:06] Well [02:06] More notably, it's now impossible to analyse OOPSes usefully. [02:06] StevenK: we'll disable the other one shortly; then update the linkification logic [02:06] We can't get the actual files or grep around to get trends etc. [02:06] we haven't finish the setup [02:07] its on a dedicated machine for scaling, and we should be able to get shell access [02:07] So Oops gets a machine and LP Jenkins doesn't? [02:12] I don't know where to even start [02:14] bigjools: does maas use package django? just python-django ? [02:15] lifeless: yeah pulls in the package [02:34] wgrant: I wonder if I can have the schema out in the class and then override the field [02:47] StevenK: Define it in the top level of the class body? That'll be a single interface for all instances of that class, so mutating it is not a good idea. [03:07] hello [03:08] wgrant: Mutating the field is a bad idea? [03:08] anybody know vala langueges? [03:08] StevenK: If the class is shared between all instances, yes. [03:10] wgrant: It's used for BranchEditWhiteboardView BranchEditStatusView BranchEditView BranchReviewerEditView [03:10] So it's probably safe to rip out and replace information_type in BranchEditView.setUpFields() [03:11] StevenK: I mean the single interface class will be shared between BranchEditView etc. instances. [03:11] So you'll be mutating semi-global state [03:11] Just like if you had class Foo: [03:11] bar = [] [03:11] And started poking in bar, all instances would see it. [03:12] wgrant: Right, I was going to leave information_type in BranchEditSchema withouth the vocab and then replace the field in BranchEditView.setUpFields() [03:12] StevenK: But you can't modify BranchEditSchema if you do it like that. [03:12] StevenK: wgrant: what are you guys cooking up? [03:12] Well, you can, but it's evil and I'll reject it :) [03:13] wgrant: I wasn't going to -- I was going to self.form_fields.omit() and so on [03:13] StevenK: If you're going to do that, you needn't move BranchEditSchema at all. [03:13] I thought you had a reason for not doing this. [03:13] the adapter broke it or something. [03:15] wgrant: Right, I have moved BranchEditSchema back [03:16] TypeError: unsupported operand type(s) for +=: 'FormFields' and 'Choice' [03:16] :-( [03:17] += Fields(foo_field) [03:19] from zope.formlib import form [03:19] RARGH [03:19] I hate that, and then form. is scattered all over the file [03:20] Hope you never have to work on bzr :) [03:20] They do it a bit. [03:20] I'm not really a huge fan. [03:20] I'm tempted to fix it in this file, but this branch is already 460 lines. [03:21] % bzr grep 'from zope.formlib import form' | wc -l [03:21] 28 [03:21] Maybe I will fix them [03:21] (In a seperate branch) [03:27] StevenK: It's a matter of opinion whether it's something that should be "fixed". [03:28] wgrant: It annoys me... It may annoy me enough to bend the codebase to my will. [03:30] how can to change gtk window background in vala langueges? Please help [03:34] wgrant, wallyworld_: https://code.launchpad.net/~stevenk/launchpad/branch-information_type-ui/+merge/107909 would appreciate a review. [03:34] /me looks [03:36] StevenK: Does that actually work? [03:37] You aren't hiding explicitly_private when the flag is set. [03:37] hbt272: this channel has nothing to do with gtk [03:37] hbt272: I think you are not getting answers because people don't know. You would be better off asking in a desktop development channel. [03:37] StevenK: Oh, you do that separately down the bottom. That's a bit odd. I'd just customise field_names based on the flag and have everything else depend on the contents of field_names. [03:38] lifeless: thank you! [03:39] wgrant: My reasoning behind that was two-fold: 1) The current rules look complex, and 2) We are going to revisit this anyway due to self-service, so thought it could stay put until then. [03:40] StevenK: Does omit break if the field isn't there? [03:43] StevenK: I'd replace the stuff in "if not show_private_field" with "self.form_fields = self.form_fields.omit('explicitly_private', 'information_type')" [03:44] And the thing which replaces the field on line 257 of the diff with "if self.form_fields.get('information_type') is not None" [03:45] That encapsulates the field selection logic in field_names. [03:48] StevenK: sorry, got pinger by thumper, looks like wgrant is doing the review [03:49] Yup [04:04] wgrant: But information_type will always be not None? [04:05] StevenK: Not if it's not there. [04:06] wgrant: information_type is always in the schema [04:08] StevenK: if 'information_type' in self.field_names, then. [04:09] wgrant: Right, that sounds sensible [04:11] wgrant: Are those your only two comments? [04:12] StevenK: http://pastebin.ubuntu.com/1014176/ [04:12] Um, can revert the move of show_information_type_in_ui; that isn't necessary any more. [04:13] Why moving the schema? [04:13] So we don't have to override the field. [04:13] We instead create the class when self.schema is evaluated. [04:13] At which time the feature flag is already in place. [04:14] But doesn't that end up with beng unable to adapt? [04:14] Same thing as BugSecrecyEditView, except with @cachedproperty to preserve identity for adaption. [04:14] *being [04:14] Ah. [04:22] wgrant: That looks excellent. [04:30] wgrant: So you approve, or now I should get wallyworld_ to review it? :-) [04:30] StevenK: Was waiting for the diff to update. [04:30] It should be done [04:30] Indeed. [04:36] * wallyworld_ laments that getPrecachedPersonsFromIDs doesn't seed the ValidPersonCache :-( [04:36] wgrant: Did you see cjwatson's MP about CustomUpload? [04:36] wallyworld_: ValidPersonCache is the most horrible thing in the world. [04:36] ... aside from my indenting. [04:37] wallyworld_: But there should be an option to getPrecachedPersonsFromIDs that cached it, I thought. [04:37] StevenK: I veeeeery nearly said that. [04:37] Hahahaha [04:37] But decided it could be mean. [04:37] wgrant: the api seems to indicate that but it doesn't work [04:38] wallyworld_: How're you accessing it that it doesn't appear to be cached? [04:38] * StevenK wonders why LP pages are taking ~ 3 minutes to refresh [04:38] StevenK: Because you have too many MPs open. [04:39] wgrant: person.is_valid_person does a ValidPersonCahce query even after calling getUtility(IPersonSet).getPrecachedPersonsFromIDs(grantees, need_icon=True, need_validity=True) [04:39] SIGH [04:39] LONGPOLL! [04:39] wallyworld_: Did you evaluate the resultset? [04:39] wgrant: Can I have a +1 now? [04:39] wallyworld_: getPrecachedPersonsFromIDs doesn't actually get precached persons from IDs. [04:40] wallyworld_: It gets a resultset which gets precached persons from IDs. [04:40] Try listifying it. [04:40] StevenK: Working on it. [04:40] A few more comments. [04:40] wgrant: i didn't evaluate it. that may be what i'm missing. will try that, thanks [04:40] We can't just delete ValidPersonCache? [04:40] StevenK: I plan to achieve its demise within the year. [04:40] For crimes against humanity [04:41] For it is an abomination whose reign of terror must be stopped. [04:43] wgrant: yes, that was the problem, thanks [04:43] wallyworld_: Great. [04:44] wallyworld_: I've fixed a few views that were doing the same thing. [04:44] Over time [04:44] Attempting to precache, but not actually doing anything of value. [04:44] i can imagine [04:44] StevenK: You have my +1 but with various comments. [04:44] * wallyworld_ is glad there was a query count test [04:44] which failed so it did its job [04:45] Yup [04:46] wallyworld_: You can add lists together, you know, right? [04:46] Rather than .insert(0, ... [04:47] StevenK: the order is preserved? [04:47] StevenK: ... says he who just used [2] :P [04:47] But yes, I wouldn't use insert() as wallyworld has. [04:47] Mostly because it's confusing doing it twice to get opposite order. [04:47] StevenK: feel free to do a drive by in your branch so long as the ordering remains as expected [04:48] StevenK: Are you looking at Colin's branch? [04:48] If not, I will. [04:48] wgrant: I have done, I'm happy with it, but given it's wide-reaching I wanted a second opinion. [04:49] I don't see a review. [04:49] https://code.launchpad.net/~cjwatson/launchpad/custom-upload-parsing/+merge/107656 [04:50] There's an MP, but no comments. [04:50] Did it get eaten by longpoll? [04:50] I haven't commented yet [04:50] I can if you wish [04:51] I shall fetch a drink and look over it. [04:51] * StevenK stabs longpoll more [04:51] Hm [04:51] MP diffs are a bit more readable with the excessive line-height disabled. [04:52] wgrant: The portlet is not used for the edit form class, but BugView and BranchView, and they *don't* implement show_information_type_in_ui [04:52] StevenK: Ah, indeed, I am stupid. [04:52] StevenK: But they should implement that. [04:52] It's only temporary, and it's going to be shorter to have it in the view classes. [04:52] And less hideous. [04:52] Just so it doesn't look utterly bonkers [04:57] wgrant: The portlet is tested elsewhere in a doctest [04:58] StevenK: Ah. That's what I would have expected if it was eg. Bugs, but I thought Code had a bit more sense than that. [04:59] Clearly not [05:00] /win 3 [05:00] Bah [05:01] wgrant: http://pastebin.ubuntu.com/1014208/ [05:03] StevenK: hat was that nbsp doing there? A leftover from the link that isn't there yet? [05:04] wgrant: It was copied from the bugs template, so likely. [05:04] Right. LGTM, then. [05:04] Thanks. [05:10] hurry up launchpad, scan my @!@$!% branch already [05:11] wallyworld_: You used to be on the code team, fix the scanner. [05:11] * StevenK hides. [05:11] * wallyworld_ gets out flamethrower [05:11] * wallyworld_ blames thumper [05:16] wallyworld_: Do you recall what AMI your ec2 runs this morning used? [05:16] StevenK: 529 i am pretty sure [05:16] That is excellent news. [05:17] hmmm. i think this branch is stuck. 15 minutes and counting [05:17] StevenK: yes, glad you fixed it [05:18] ah, just completed [05:18] wallyworld_: I'm still a bit unhappy that it took me until 1:10am to do so. [05:19] StevenK: take the last hour or so of today off. have a beer :-) [05:19] ENOBEER [05:19] wine then :-) [05:20] ENOWINE [05:20] ENOALCOHOL [05:22] * StevenK gets tempted to fix ec2 images to not use $EMAIL [05:22] Actually how does even *work* [05:23] you have no alcohol in the house? shame [05:23] what, is that a whip i can hear? [05:32] * StevenK stabs buildbot [05:36] Usual bzr thing? [05:36] No, my fault [05:36] No [05:36] This looks legit. [05:36] Yeah [05:37] I have a small fix [05:42] wgrant: https://code.launchpad.net/~stevenk/launchpad/testfix-no-builder-description/+merge/107913 [05:44] StevenK: If it works, ship it. [05:44] wgrant: Did you look at cjwatson's MP? [05:47] StevenK: Most of the way there. It's the sort of thing I'd tend to self-review just to avoid subjecting a reviewer to it. [05:49] Yah, self-reviewed and landed [05:49] No, I mean cjwatson's thing. [05:49] But yes, yours was obviously self-reviewable :) [05:50] Ah [05:53] StevenK: It has my +1 [06:08] wgrant: Excellent [06:51] good morning === almaisan-away is now known as al-maisan [06:51] hi al-maisan [06:51] moin adeuring [06:51] how are things? === al-maisan is now known as almaisan-away [07:00] wgrant: Hmm. I did initially try a classmethod for this custom upload rearrangement and it didn't seem to come out shaped very nicely; in particular I definitely found moving the target directory stuff out of __init__ to be a win (Jeroen suggested consolidating *AlreadyExists on the bug, and I never liked the way that it sometimes failed in the constructor and sometimes in process anyway) [07:00] But I can try again if you like, maybe with just the classmethod change but leaving __init__ as I currently have it? [07:01] cjwatson: Mm, that's true, it could fail in setTargetDirectory if it's badly formatted. So maybe. [07:02] Anyway, it's just a suggestion. === frankban changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On call reviewer: frankban* (rvba) | Firefighting: - | Critical bugs: 3.47*10^2 [08:18] Could I have "Status: Approve" on https://code.launchpad.net/~cjwatson/launchpad/queue-api-accept-reject/+merge/107894 so I can land it? It has a +1 review already. [08:19] cjwatson: Sorry about that. [08:19] cjwatson: Done. [08:19] NP, thanks [08:19] Is scripts/ftpmaster-tools/queue dead yet? :-) [08:20] ... a bit of work left for that [08:21] I know :-) [08:32] cjwatson: You probably want to make QueueInconsistentStateError a 400 rather than an OOPS [08:32] cjwatson: The browser code catches it and turns it into a non-OOPS error, but the API will default to OOPSing. [08:34] wgrant: Hm. I've already started landing this. Should I abort? [08:34] (I agree) [08:35] cjwatson: No, the OOPS reports are shit enough already that nobody will notice. [08:35] But a quick followup would be nice. [08:35] That said I didn't start ec2 land that long ago. Probably more economical to kill it. [08:35] Potentially. [08:36] * cjwatson does so. [08:54] wgrant: So http://bazaar.launchpad.net/~cjwatson/launchpad/queue-api-accept-reject/revision/15326 ? [09:02] can someone please land this approved branch for me? [09:02] https://code.launchpad.net/~jml/launchpad/archive-commercial-rename-support/+merge/107819 [09:02] hmm, wait, ec2 land works for me, I think. [09:02] never mind. [09:04] cjwatson: Looks good. [09:07] wgrant: Thanks. Landing for real now. [09:09] Great. [09:10] Oh [09:10] ECHAN [10:08] wgrant: from #launchpad, so what would be the steps if we couldn't get a branch owner to poke things themselves? [10:08] the same but a l-osa? [10:10] mgz: there used to be a team that had write privs to all branches. I guess any administrator could still do that. [10:12] mgz: Right, webops have write access to all branches. [10:12] So they can carry out the same actions, and it's sometimes easier to do that than walk the user through it. [10:16] It looks like the relevant commit was directly to trunk, so hopefully none of the stacked-on branches have a copy of the corruption. [10:20] jml: That team (bazaar-experts) is long dead. [10:41] wgrant: I tweaked https://code.launchpad.net/~cjwatson/launchpad/custom-upload-parsing/+merge/107656 a bit; if that looks OK to you now, could I have "Status: Approved"? [10:43] cjwatson: Thanks. Done. [11:30] mgz: The example invocation in the script is out of date. [11:30] mgz: The statement in the bug is correct. [11:30]