=== salgado is now known as salgado-afk === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [15:02] Well [15:02] * sinzui stumbles his way to the chair [15:03] == Agenda == [15:03] * Roll call [15:03] * Next meeting [15:03] * Action items [15:03] * Queue status [15:03] * Mentoring update [15:03] * Review process [15:03] * (intellectronica) JS-related templates should be tested (as much as possible). [15:03] * (barry) don't assign GQ after on-call session, let next ocr take them [15:03] Welcome to this week's AMEU reviewers meeting [15:04] * sinzui wonders if he is alone [15:04] me [15:04] me [15:04] me [15:04] me [15:04] you're not! [15:04] me [15:04] me [15:04] me [15:05] I think we were waiting for the roll call! [15:05] schwuk sends his apologies [15:05] bigjools: so was I, yet I expect a few premature mes [15:05] his wife is ill so he had to do the school run [15:05] barry and gmb are excused [15:06] we seem to be short a few people [15:06] EdwinGrubbs: ping [15:06] me [15:06] me [15:07] danilos: ping [15:07] me [15:07] flacoste: thanks [15:07] == Next meeting == [15:07] Today +7 days? === jt1 is now known as jtv [15:07] me [15:08] Silence is compliance. [15:09] silence is golden [15:09] Next meeting of the AMEU reviewers is in 7 days. [15:09] == Agenda == [15:09] * Roll call [15:09] * Next meeting [15:09] * Action items [15:09] * Queue status [15:09] * Mentoring update [15:09] * Review process [15:10] * (intellectronica) JS-related templates should be tested (as much as possible). [15:10] * (barry) don't assign GQ after on-call session, let next ocr take them [15:10] bugger [15:10] === Outstanding Actions === [15:10] * intellectronica to file bug on lint issue regarding elementtree import [15:10] I have not seen this gub [15:10] sorry, i haven't done it yet [15:11] Shall I keep this item for next week? [15:11] * mwhudson to start discussion on page test purpose [15:11] I have not seen this discussion. I look forward to it. [15:12] sinzui: please, yes [15:12] * thumper to submit a bug for moving ftests contents to tests [15:12] /me has not see this bug either [15:13] * gmb to update PythonStyleGuide for long argument lists (use The Salgado Way) [15:13] gmb did complete this [15:13] * barry to ask lifeless to summarize what he knows about the PQM Mysteries (e.g. autopacking bug losing branches) [15:13] Not done. [15:13] * jml to find out how divmodders test their javascript [15:13] Not done [15:13] === Proposed items === * Include file-by-file summary in review template (schwuk) [15:14] it's a nice thing to do if you can, but i think demanding it for every submission would be cruel [15:14] schwuk: are you proposing a change to the cover letter in lpreview? [15:14] i think he's not here?... [15:15] That is right [15:15] yeah for large branch it makes sense [15:15] * sinzui drinks more coffee [15:15] but i don't think making this mandatory is a good idea [15:15] A summary of each file is nice, but I sometimes do it by feature/fix [15:17] I think the reason we might want a summary by file is because there changes outside the feature/bug that were made, or the connection to the feature was non-obvious [15:18] If that is the case, we might just remind the developer in the cover letter template that the non-obvious changes should be listed [15:18] "Go through your diff and..." [15:18] That is what I do [15:19] * sinzui has written novellas. [15:19] for big changes it really helps [15:19] we do have a cover letter template for non-trivial submissions [15:20] perhaps we should encourage people to you use it [15:20] me included, i don't i've used it more than once or twice [15:20] i think a summary per file on works for certain type of changes. often, if you write it per-file, the cover letter won't flow as nicely as if you write one summary for all files. [15:20] * sinzui agrees with BjornT [15:20] where certain type = big? [15:21] having the modified files listed in the template would help with remembering non-related changes [15:21] when I write my cover letters I usually have the diff open in another window to remind me what to say [15:21] Changes that are subtle, spanning many files might not be big [15:22] common sense should prevail [15:22] bigjools: not necessarily. it's more like if you do non-related changes (which we shouldn't encourage people to do) [15:22] There are also changes that cascade though tests and templates. [15:23] it really depends on the change - it should be encouraged if it's a complicated change, otherwise it's not so useful [15:24] Should regard listing changes as a best practice when the reason for the change are not obvious? [15:24] s/should/Should we/ [15:25] something like that, yes [15:26] Do we document that on the wiki, or amend the cover letter template in review-submit [15:27] i don't think we should make this change in review-submit [15:27] * sinzui favours a wiki addition and an email to launchpad. [15:27] i can amend the cover letter template on the wiki, and we can remind devs that it is a good reference for an elaborate cover letter, when the situation calls for one [15:28] I think that is a good start [15:28] me [15:28] sorry - got held up [15:29] schwuk: we were concluding your proposal to Include file-by-file summary in review template [15:29] sinzui: just been catching up [15:31] sinzui: I think everything has been covered, so special to add. The per-file summary helps on large changes, but there should still be a general summary of the changes. [15:31] schwuk: thank you. I agree [15:32] == Queue status == [15:32] * sinzui refreshes the page [15:32] sinzui: ftr there's an action for me, above [15:33] sinzui: intellectronica to amend cover letter template to include a per-file summary section and write an email to the list [15:33] intellectronica: thank you for reminding me of my responsabilities [15:33] I will update the agenda for next week to include this [15:34] I think the queue looks fine. mars needs to find a reviewer I think [15:35] yep [15:35] schwuk: you have a missing branch in merge-conditional status. [15:35] i probably won't take any more reviews today after what i've got in the queue, so there may be some time on the gq at day's end [15:35] don't know about allenap [15:35] schwuk: did you land it and remove the directory? [15:36] If I have time left, I'll look at the general queue for things to do. [15:36] sinzui: possibly - I cleaned up branches on devpad when they were complaining about space [15:37] salgado: you have broken up Mark's branch. Should it be in needs-review? [15:37] sinzui, no, I'll remove it from there [15:37] sinzui: That branch was landed. [15:38] schwuk: thank you [15:38] Does anyone have anything else to say about the queue? [15:39] == Mentoring update == [15:39] Does anyone have something to say about this? [15:40] == Review process == [15:40] * (intellectronica) JS-related templates should be tested (as much as possible). [15:40] so [15:40] we don't test javascript code [15:40] but there's quite a lot we can do in our normal test suite [15:41] by testing the interaction between javascript and templates, for example [15:41] testing for the IDs of elements, testing for the presence of elements within a deeper structure, testing for the presence of a css class, etc'... [15:42] testing that the script is present, that is contains the expected functions? [15:42] sinzui: indeed [15:42] also, make sure you run the ui locally and watch for javascript errors and the behaviour of javascript code [15:43] and use FireBug ;) [15:43] finally, make sure you read javascript code with the same critical eye you read python code with [15:43] if you're unsure about how something works, ask questions [15:43] firebug is not FF3 compatible [15:43] intellectronica: some browsers are also able to show warnings & errors in JS. Maybe test with those? [15:44] * sinzui has fallen back to developer tools [15:44] think about defensive code. how can it fail? what will happen when it does? has everything been done to mitigate the effects of a failure [15:44] sinzui: there's an ubuntu package with a working firebug, mainained by asac [15:45] i'm the self-appointed guardian angel of Safari. if you review a branch with lots of JS changes let me know and i'll be glad to test the UI in Safari. [15:45] jtv: i think all browsers can show that, to some extent. but the most important thing is simply to look for breakage [15:45] bac, cool, that will help [15:45] sinzui: there's a beta of firebug that works with FF3 [15:45] bac: great to know. i think allenap and mpt too have an apple close to hand [15:45] My concern about testing for ids and classes, is that those might be variables in the script. So we are reduced to knowing that a script demands an id or class, and checking the browser.content [15:46] sinzui: perhaps we should add some helper functions to make it easier to test things in