| === salgado is now known as salgado-afk | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| sinzui | Well | 15:02 |
|---|---|---|
| * sinzui stumbles his way to the chair | 15:02 | |
| sinzui | == Agenda == | 15:03 |
| sinzui | * Roll call | 15:03 |
| sinzui | * Next meeting | 15:03 |
| sinzui | * Action items | 15:03 |
| sinzui | * Queue status | 15:03 |
| sinzui | * Mentoring update | 15:03 |
| sinzui | * Review process | 15:03 |
| sinzui | * (intellectronica) JS-related templates should be tested (as much as possible). | 15:03 |
| sinzui | * (barry) don't assign GQ after on-call session, let next ocr take them | 15:03 |
| sinzui | Welcome to this week's AMEU reviewers meeting | 15:03 |
| * sinzui wonders if he is alone | 15:04 | |
| salgado | me | 15:04 |
| intellectronica | me | 15:04 |
| bac | me | 15:04 |
| bigjools | me | 15:04 |
| salgado | you're not! | 15:04 |
| sinzui | me | 15:04 |
| allenap | me | 15:04 |
| BjornT | me | 15:04 |
| bigjools | I think we were waiting for the roll call! | 15:05 |
| bigjools | schwuk sends his apologies | 15:05 |
| sinzui | bigjools: so was I, yet I expect a few premature mes | 15:05 |
| bigjools | his wife is ill so he had to do the school run | 15:05 |
| sinzui | barry and gmb are excused | 15:05 |
| sinzui | we seem to be short a few people | 15:06 |
| bac | EdwinGrubbs: ping | 15:06 |
| flacoste | me | 15:06 |
| EdwinGrubbs | me | 15:06 |
| flacoste | danilos: ping | 15:07 |
| danilos | me | 15:07 |
| danilos | flacoste: thanks | 15:07 |
| sinzui | == Next meeting == | 15:07 |
| sinzui | Today +7 days? | 15:07 |
| === jt1 is now known as jtv | ||
| jtv | me | 15:07 |
| sinzui | Silence is compliance. | 15:08 |
| intellectronica | silence is golden | 15:09 |
| sinzui | Next meeting of the AMEU reviewers is in 7 days. | 15:09 |
| sinzui | == Agenda == | 15:09 |
| sinzui | * Roll call | 15:09 |
| sinzui | * Next meeting | 15:09 |
| sinzui | * Action items | 15:09 |
| sinzui | * Queue status | 15:09 |
| sinzui | * Mentoring update | 15:09 |
| sinzui | * Review process | 15:09 |
| sinzui | * (intellectronica) JS-related templates should be tested (as much as possible). | 15:10 |
| sinzui | * (barry) don't assign GQ after on-call session, let next ocr take them | 15:10 |
| sinzui | bugger | 15:10 |
| sinzui | === Outstanding Actions === | 15:10 |
| sinzui | * intellectronica to file bug on lint issue regarding elementtree import | 15:10 |
| sinzui | I have not seen this gub | 15:10 |
| intellectronica | sorry, i haven't done it yet | 15:10 |
| sinzui | Shall I keep this item for next week? | 15:11 |
| sinzui | * mwhudson to start discussion on page test purpose | 15:11 |
| sinzui | I have not seen this discussion. I look forward to it. | 15:11 |
| intellectronica | sinzui: please, yes | 15:12 |
| sinzui | * thumper to submit a bug for moving ftests contents to tests | 15:12 |
| sinzui | /me has not see this bug either | 15:12 |
| sinzui | * gmb to update PythonStyleGuide for long argument lists (use The Salgado Way) | 15:13 |
| sinzui | gmb did complete this | 15:13 |
| sinzui | * barry to ask lifeless to summarize what he knows about the PQM Mysteries (e.g. autopacking bug losing branches) | 15:13 |
| sinzui | Not done. | 15:13 |
| sinzui | * jml to find out how divmodders test their javascript | 15:13 |
| sinzui | Not done | 15:13 |
| sinzui | === Proposed items === * Include file-by-file summary in review template (schwuk) | 15:13 |
| intellectronica | it's a nice thing to do if you can, but i think demanding it for every submission would be cruel | 15:14 |
| sinzui | schwuk: are you proposing a change to the cover letter in lpreview? | 15:14 |
| intellectronica | i think he's not here?... | 15:14 |
| sinzui | That is right | 15:15 |
| flacoste | yeah for large branch it makes sense | 15:15 |
| * sinzui drinks more coffee | 15:15 | |
| flacoste | but i don't think making this mandatory is a good idea | 15:15 |
| sinzui | A summary of each file is nice, but I sometimes do it by feature/fix | 15:15 |
| sinzui | 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:17 |
| sinzui | 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 |
| jtv | "Go through your diff and..." | 15:18 |
| sinzui | That is what I do | 15:18 |
| * sinzui has written novellas. | 15:19 | |
| bigjools | for big changes it really helps | 15:19 |
| intellectronica | we do have a cover letter template for non-trivial submissions | 15:19 |
| intellectronica | perhaps we should encourage people to you use it | 15:20 |
| intellectronica | me included, i don't i've used it more than once or twice | 15:20 |
| BjornT | 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 | |
| bigjools | where certain type = big? | 15:20 |
| BjornT | having the modified files listed in the template would help with remembering non-related changes | 15:21 |
| bigjools | when I write my cover letters I usually have the diff open in another window to remind me what to say | 15:21 |
| sinzui | Changes that are subtle, spanning many files might not be big | 15:21 |
| bigjools | common sense should prevail | 15:22 |
| BjornT | bigjools: not necessarily. it's more like if you do non-related changes (which we shouldn't encourage people to do) | 15:22 |
| sinzui | There are also changes that cascade though tests and templates. | 15:22 |
| bigjools | it really depends on the change - it should be encouraged if it's a complicated change, otherwise it's not so useful | 15:23 |
| sinzui | Should regard listing changes as a best practice when the reason for the change are not obvious? | 15:24 |
| sinzui | s/should/Should we/ | 15:24 |
| intellectronica | something like that, yes | 15:25 |
| sinzui | Do we document that on the wiki, or amend the cover letter template in review-submit | 15:26 |
| intellectronica | 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 | |
| intellectronica | 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:27 |
| sinzui | I think that is a good start | 15:28 |
| schwuk | me | 15:28 |
| schwuk | sorry - got held up | 15:28 |
| sinzui | schwuk: we were concluding your proposal to Include file-by-file summary in review template | 15:29 |
| schwuk | sinzui: just been catching up | 15:29 |
| schwuk | 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 |
| sinzui | schwuk: thank you. I agree | 15:31 |
| sinzui | == Queue status == | 15:32 |
| * sinzui refreshes the page | 15:32 | |
| intellectronica | sinzui: ftr there's an action for me, above | 15:32 |
| intellectronica | sinzui: intellectronica to amend cover letter template to include a per-file summary section and write an email to the list | 15:33 |
| sinzui | intellectronica: thank you for reminding me of my responsabilities | 15:33 |
| sinzui | I will update the agenda for next week to include this | 15:33 |
| sinzui | I think the queue looks fine. mars needs to find a reviewer I think | 15:34 |
| mars | yep | 15:35 |
| sinzui | schwuk: you have a missing branch in merge-conditional status. | 15:35 |
| intellectronica | 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 |
| intellectronica | don't know about allenap | 15:35 |
| sinzui | schwuk: did you land it and remove the directory? | 15:35 |
| allenap | If I have time left, I'll look at the general queue for things to do. | 15:36 |
| schwuk | sinzui: possibly - I cleaned up branches on devpad when they were complaining about space | 15:36 |
| sinzui | salgado: you have broken up Mark's branch. Should it be in needs-review? | 15:37 |
| salgado | sinzui, no, I'll remove it from there | 15:37 |
| schwuk | sinzui: That branch was landed. | 15:37 |
| sinzui | schwuk: thank you | 15:38 |
| sinzui | Does anyone have anything else to say about the queue? | 15:38 |
| sinzui | == Mentoring update == | 15:39 |
| sinzui | Does anyone have something to say about this? | 15:39 |
| sinzui | == Review process == | 15:40 |
| sinzui | * (intellectronica) JS-related templates should be tested (as much as possible). | 15:40 |
| intellectronica | so | 15:40 |
| intellectronica | we don't test javascript code | 15:40 |
| intellectronica | but there's quite a lot we can do in our normal test suite | 15:40 |
| intellectronica | by testing the interaction between javascript and templates, for example | 15:41 |
| intellectronica | 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:41 |
| sinzui | testing that the script is present, that is contains the expected functions? | 15:42 |
| intellectronica | sinzui: indeed | 15:42 |
| intellectronica | also, make sure you run the ui locally and watch for javascript errors and the behaviour of javascript code | 15:42 |
| mars | and use FireBug ;) | 15:43 |
| intellectronica | finally, make sure you read javascript code with the same critical eye you read python code with | 15:43 |
| intellectronica | if you're unsure about how something works, ask questions | 15:43 |
| sinzui | firebug is not FF3 compatible | 15:43 |
| jtv | intellectronica: some browsers are also able to show warnings & errors in JS. Maybe test with those? | 15:43 |
| * sinzui has fallen back to developer tools | 15:44 | |
| intellectronica | 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 |
| intellectronica | sinzui: there's an ubuntu package with a working firebug, mainained by asac | 15:44 |
| bac | 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 |
| intellectronica | jtv: i think all browsers can show that, to some extent. but the most important thing is simply to look for breakage | 15:45 |
| mars | bac, cool, that will help | 15:45 |
| schwuk | sinzui: there's a beta of firebug that works with FF3 | 15:45 |
| intellectronica | bac: great to know. i think allenap and mpt too have an apple close to hand | 15:45 |
| sinzui | 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:45 |
| intellectronica | sinzui: perhaps we should add some helper functions to make it easier to test things in <script> | 15:46 |
| intellectronica | best is probably just to add such functions when the need arises, anyway | 15:46 |
| mpt | I don't usually bring my Mac to work any more | 15:46 |
| sinzui | intellectronica: I was thinking of extracting the getByID and $() to make a list of items to look for in the content | 15:47 |
| intellectronica | b.t.w it's possible to run both IE and Safari on ubuntu, if you're curious | 15:47 |
| sinzui | The mac is not important. Wenkit is | 15:47 |
| sinzui | WebKit is | 15:47 |
| sinzui | we can used the next gen of epiphany | 15:48 |
| intellectronica | anyway, i don't think we have too much trouble with platform/coverage | 15:48 |
| intellectronica | i think it's more a matter of reviewing the javascript code itself | 15:48 |
| bigjools | we're over time - is there much more? | 15:49 |
| intellectronica | and reviewing templates, which may be related to javascript code, even when it isn't obvious | 15:49 |
| sinzui | bigjools: one more item | 15:49 |
| mpt | sinzui, or Midori | 15:50 |
| mpt | (which has the advantage of being in the Hardy repository already) | 15:50 |
| sinzui | mpt: ! I did not know that | 15:50 |
| sinzui | intellectronica: I think we need a list of what to review. Checking for how/where a script connects to the page is doable by all of us. | 15:51 |
| intellectronica | sinzui: i'm happy to compile a list and put it on the wiki | 15:52 |
| sinzui | knowing when to use a library (like Mochikit) instead of doing writing your own block/function is another matter | 15:52 |
| mars | sinzui, yes, but that should be taken care of in the pre-implementation call, no? | 15:53 |
| intellectronica | sinzui: yeah, that could all be described as "is this the best way to write this feature" | 15:53 |
| intellectronica | not much different from python. it's just that we need to make sure we pay attention to this in the review | 15:54 |
| sinzui | intellectronica: Please compile the list, something is better than nothing. It will definitely help prevent a bug from some months ago that happened when the id the script required was not present. | 15:54 |
| intellectronica | and the one from last week ;) | 15:54 |
| intellectronica | sinzui: will do | 15:55 |
| sinzui | mars: it should, I think the issue is we do not have the JS experience in reviewing that we need given the script that has been written this year | 15:56 |
| sinzui | * (barry) don't assign GQ after on-call session, let next ocr take them | 15:57 |
| sinzui | barry is not here to explain his command. I intend to honor it. | 15:57 |
| sinzui | I don't have anything to say on this matter. If know one else has, I think this meeting is finished. | 15:57 |
| sinzui | 5 | 15:57 |
| sinzui | 4 | 15:57 |
| sinzui | 3 | 15:57 |
| sinzui | 2 | 15:57 |
| sinzui | 1 | 15:57 |
| sinzui | So ends the meeting | 15:57 |
| intellectronica | thanks sinzui, great chairing | 15:58 |
| schwuk | Thanks sinzui | 15:58 |
| sinzui | Thank you all for playing | 15:58 |
| bigjools | thanks sinzui | 15:58 |
| === salgado is now known as salgado-lunch | ||
| === salgado-lunch is now known as salgado | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!