| === abentley1 is now known as abentley | ||
| === Ursinha is now known as Ursinha-sprint | ||
| al-maisan | Good morning! | 08:42 |
|---|---|---|
| === intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: jtv || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com | ||
| === intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| intellec` | gmb: i wonder if a name like forceUpdateAll or something like that would be more appropriate for the method you've added. it's a bit confusing to read that there are no watches to update, and yet when you call updateAllWatches some update | 10:02 |
| === Ursinha is now known as Ursinha-sprint | ||
| === intellec` is now known as intellectronica | ||
| gmb | intellectronica: Hmm. Yes, I see your point. forceUpdateAll() also lends the method an air of danger, which it posesses in spades :) | 10:08 |
| gmb | intellectronica: I'll change the method name. | 10:08 |
| intellectronica | cool | 10:09 |
| intellectronica | gmb: r=me with that change. nice branch! | 10:27 |
| gmb | intellectronica: Thanks :) | 10:38 |
| BjornT | gmb: i have a few questions about that review | 11:26 |
| BjornT | gmb: what happens if you 1) run checkwatches.py --all 2) Ctrl-C:s since it's taking too long 3) restart checkwatches.py without the --all option? | 11:28 |
| gmb | BjornT: If you run checkwatches.py --all without specifying a bugtracker it'll be ignored, for a start. If you kill it in the middle of a run then the watches will be left with lastchecked=None, because checkwatches deliberately just re-raises KeyboardInterrupt and SystemExit exceptions. | 11:31 |
| BjornT | gmb: right, that's what i thought (regarding bug watches being left with lastchecked=None) | 11:32 |
| gmb | BjornT: So I guess what we could do is have Ctrl-C mark the watches as lastchecked=datetime.now(), which would stop them from being updated next time. That seems a bit kludgy though. Alternatively we could keep a mapping of watch id => lastchecked time and just restore the old data. | 11:32 |
| gmb | Or, hey, we could roll back :) | 11:32 |
| * gmb has just checked where the transaction boundaries lie. | 11:33 | |
| gmb | Hmm. | 11:33 |
| BjornT | gmb: it's nothing i would expect from using --all (and no, you can't roll back, since you commit the transaction after resetting) | 11:33 |
| gmb | BjornT: No, you right, we can't. Thought it was in the try: except block. | 11:33 |
| gmb | BjornT: So, the simple solution is setting lastchecked=datetime.now() for all watches. | 11:34 |
| BjornT | gmb: i think the behaviour is fine, just unexpected. i think renaming --all to --reset would make things clearer. then it's more explicit | 11:34 |
| gmb | BjornT: Okay, I agree with you. | 11:34 |
| gmb | I'll make that change now. Thanks. | 11:34 |
| BjornT | gmb: thanks. also, don't we run checkwatches.py in some test already? | 11:34 |
| gmb | BjornT: Yes, we do, but I wanted to demonstrate that this could be done from the command line. | 11:35 |
| BjornT | gmb: you shouldn't do that by starting a sub process. the LaunchpadScript class allows you to pass in arguments to it for testing | 11:36 |
| gmb | BjornT: Oh, okay. Didn't know that. I'll update the test to use that, then. | 11:36 |
| BjornT | gmb: that way you can make the test more useful, instead of checking that passing --all has the same effect as not passing --all ;) | 11:37 |
| gmb | :) | 11:37 |
| gmb | Righto. | 11:37 |
| gmb | BjornT: Hang on, though, how do I import from cronscripts/checkwatches? I thought cronscripts wasn't in the PYTHONPATH. | 11:39 |
| BjornT | gmb: you don't. you move the CheckWatches class to somewhere you can import it. | 11:40 |
| gmb | BjornT: Ah, right. Figured you might say that :) | 11:40 |
| === Ursinha-sprint is now known as Ursinha-afk | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === Ursinha-afk is now known as Ursinha-sprint | ||
| matsubara | ow | 13:34 |
| matsubara | oops | 13:34 |
| === abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === stub1 is now known as stub | ||
| === jtatum` is now known as jtatum | ||
| abentley | intellectronica: Could you please review https://code.launchpad.net/~abentley/launchpad/suppress-resubmit/+merge/12872 ? | 15:31 |
| intellectronica | abentley: sure | 15:32 |
| abentley | intellectronica: Thanks. | 15:32 |
| intellectronica | abentley: so what's the story with those lint warnings? | 15:33 |
| abentley | intellectronica: Dunno. Bogus lint warnings like that have been showing up for months. | 15:35 |
| intellectronica | abentley: in wonder if it would be better to move the check into findRelatedBMP | 15:35 |
| intellectronica | fair enough | 15:35 |
| abentley | intellectronica: Well, it would be findRelatedAndNotSupersededBMP then. | 15:36 |
| intellectronica | no, i meant to also parameterize it | 15:36 |
| intellectronica | but now i see that it's not in the interface. if it's just an internal function then i guess it doesn't really matter much | 15:36 |
| abentley | intellectronica: I'd be happy to change it to something like findRelatedBMP(include_superseded=False) | 15:37 |
| intellectronica | abentley: yes, that's what i meant. your call | 15:38 |
| abentley | intellectronica: I've pushed a version that does that. Here's the diff: http://pastebin.ubuntu.com/286266/ | 15:56 |
| intellectronica | abentley: great. r=me | 15:58 |
| abentley | intellectronica: Thanks. | 15:58 |
| === deryck is now known as deryck[lunch] | ||
| allenap | abentley: Can you review a very small trivial wording change for me? https://code.edge.launchpad.net/~allenap/launchpad/just-comment-on-question-bug-114710-wording-change/+merge/12879 | 16:40 |
| abentley | allenap: sure | 16:41 |
| abentley | allenap: r=me. | 16:42 |
| allenap | abentley: Thanks. | 16:42 |
| === sinzui1 changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb, - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com | ||
| intellectronica | sinzui1: what do you need reviewed | 16:48 |
| intellectronica | ? | 16:48 |
| sinzui1 | intellectronica: I do | 16:48 |
| sinzui1 | https://code.edge.launchpad.net/~sinzui/launchpad/delete-structural-target/+merge/12877 | 16:48 |
| intellectronica | oright, i'm reviewing it | 16:48 |
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| intellectronica | sinzui1: are the changes to launchpad-login.pt and style-3-0.css really part of what you want me to review? | 16:53 |
| === matsubara is now known as matsubara-lunch | ||
| intellectronica | sinzui1: also, isn't destorySelf an sqlobjectism? | 16:55 |
| intellectronica | sinzui1: also, does it really make sense to disallow the deletion of a milestone with subscribers? anyone can subscribe to the milestone, so this can make it very inconvenient for the project team. i'd expect to be able to delete it with the subscriptions deleted too | 16:58 |
| intellectronica | sinzui1: ah, i think i understand what's going on. you're doing that, but in the view. the problem is that the behaviour is then different if you interact with the model directly (through the API, for example) | 17:01 |
| === Ursinha is now known as Ursinha-afk | ||
| intellectronica | sinzui1: also nm my first question. i now see why you did want me to review those changes | 17:02 |
| intellectronica | sinzui1: so the code looks fine, but i really think the work should be done in the model and not in the view. what do you think? | 17:03 |
| sinzui1 | intellectronica: yes, the login style fix is required because .subordinate must work as beuno wants | 17:04 |
| intellectronica | yeah got it once i read the rest of the diff | 17:04 |
| sinzui1 | intellectronica: destorySelf may be a sqlobject-ism, but that is also the method we use for all destroys. I do not know of another | 17:05 |
| intellectronica | sinzui1: the storm way of doing this is store.remove(obj) | 17:05 |
| intellectronica | or something like that | 17:05 |
| sinzui1 | intellectronica: I can try that | 17:05 |
| intellectronica | sinzui1: Store.of(obj).remove(obj) | 17:07 |
| intellectronica | a bit clumsier, but it will be nice to slowly get rid of the sqlobject stuff in the codebase | 17:07 |
| sinzui1 | intellectronica: I am skeptical of moving the product series work into the model. 1) it is not true delete, 2, it couple the model to a dozen objects, some of which are outside the registry | 17:07 |
| intellectronica | sinzui1: right, i understand the dilemma | 17:08 |
| intellectronica | how would we solve that if and when we want to expose that functionality via the API? | 17:09 |
| sinzui1 | We do not intend to expose this over the API | 17:09 |
| sinzui1 | When we can delete translations, we ill consider it | 17:09 |
| === sinzui1 is now known as sinzui | ||
| intellectronica | that's a good answer, i guess :) | 17:09 |
| sinzui | intellectronica: danilo thinks it is possible but this kind of issue is not out focus anymore | 17:10 |
| sinzui | :( | 17:10 |
| intellectronica | though i can totally see how milestone and series mgmt is something project teams might want to automate | 17:10 |
| intellectronica | yeah, i realise that :-/ | 17:10 |
| intellectronica | oh well, hopefully one day we'll have an opportunity to improve on that | 17:11 |
| intellectronica | sinzui: for now, r=me | 17:11 |
| sinzui | thanks. I will work on the storm change now | 17:11 |
| intellectronica | cool | 17:11 |
| === intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| sinzui | intellectronica: thanks for the storm suggestion. I was able to factor out removeSecurityProxy. | 17:38 |
| intellectronica | excellent! | 17:39 |
| === deryck[lunch] is now known as deryck | ||
| === intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === beuno is now known as beuno-lunch | ||
| === matsubara-lunch is now known as matsubara | ||
| === beuno-lunch is now known as beuno | ||
| === jamalta is now known as jamalta-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| === matsubara_ is now known as matsubara-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!