[01:11] hmm, I wants areviewer [01:11] https://code.launchpad.net/~lifeless/launchpad/cachedproperty/+merge/32728 [01:11] anyone up for it ? [01:11] mwhudson: not that I'm stalking you or anything .... pretty please? [01:11] * mwhudson looks [01:16] lifeless: i guess you grepped for existing __storm_invalidate__ methods so i don't have to? :) [01:16] yeah [01:16] in devel as of latest commit a couple of hours ago [01:16] of course, folk can still add them [01:17] so it needs a psg update, mail to the list, various repeats etc etc [01:20] lifeless: approved [01:21] \o/ [01:23] mwhudson: click the other button too [01:23] (please) [01:23] lifeless: done [01:23] lifeless: can't you do that though? [01:24] mwhudson: I can, but its a pita :) [01:24] ok [01:24] so I try to do it for other people when I review their branches [01:24] so that when they are told approved, they just need to do 'ec2 land'... [01:24] rather than reopen the web page, etc etc [01:25] anyhow thanks heaps for the review [01:25] sorry if that last bit came across grumpy [01:25] its just high friction [01:27] yeah, it was a slip on my part i agree [01:27] thanks for the prod [01:40] hah, so the SQLBase super() call is bogus [01:40] because the base class doesn't have one, so I'll nuke that :) [01:41] (the test suite does blow up, I just hadn't run model class tests ;P) [02:51] mwhudson: around? I'm about to pop in a fix or two to that branch (nonobvious in foresight, clear in hindsight) [03:00] mwhudson: rev 11352 and 11353 on lp:~lifeless/launchpad/cachedproperty [03:02] lifeless: looking [03:03] thanks! [03:06] lifeless: looks fine [03:06] thanks [03:06] the need for 11353 is a bit depressing [03:06] yes [03:06] hm [03:06] oh nm [03:06] OTOH its good that folk are *already* using cachedproperty :P [03:06] was thinking that maybe you could write this as self.cachedprop.cache(value) [03:06] but obviously not [03:07] because of what self.cachedprop does :) [03:08] yes [03:08] it would be lovely [03:08] but sadly, DENIED [03:08] it might be an idea to make _cachedproperties a dict [03:08] and then clear_properties can just replace the dict [03:09] but since its now nicely abstracted ('nicely') we can do that later. [03:09] well, i guess you can do self.__class__.cachedprop [03:10] I don't think so [03:10] when the decorator is invoked you get a bare function [03:10] no im_class attribute [03:10] and definitely no instances (instances being what have __class__) [03:10] oh, I see [03:11] uhm, yes, we could make it into a obj with methods that you access via self.__class__.publicname [03:11] self.__class__.publicname.clear(self) [03:11] it a tad ugly [03:12] yeah, i think it's a bit ugly [03:13] so let's stick with what you have for now === henninge_ is now known as henninge === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [08:49] Bring on your branches! === StevenK changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:13] henninge: Hi! :-) [10:19] henninge: hi, I've made some changes to a branch sinzui approved, how do you feel about an incremental review ? [10:20] lifeless: Unless it's urgent I'd like you to take it back to sinzui, actually .. ;-) [10:21] henninge: well, given ec2 takes 4 hours to land stuff, I was hoping to throw it in overnight :) [10:21] ah right, forgot were you are ... ;) [10:21] :P [10:22] lifeless: put it on the queue [10:23] https://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/32067 === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === lifeless changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [StevenK, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:23] henninge: there is one commit in particular I want to discuss - http://bazaar.launchpad.net/~lifeless/launchpad/registry/revision/11324 [10:23] which is not domain specific [10:23] when you're done with mr K [10:23] ok ;) [10:24] Mine is only a widdle branch [10:24] StevenK: Hi! Looking ; [10:27] StevenK: I seem to remember that we voted in favor of lifeless' suggestion about on-import-per-line. Unless I am mistaken, could you please restructure the import lines that you touched? [10:27] StevenK: other than that, r=me ;-) [10:27] we did, imports are now like any other list. [10:28] well, nearly - still alpha-sorted please :) [10:29] henninge: so, my branch looks big, but all the bulky stuff is already reviewed === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lifeless || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:30] * henninge looks [10:30] I've explained in the review page the incremental stuff I would love to have more eyeballs on [10:31] danilos: the branch is ready for review btw. [10:31] henninge, cool, thanks, I'll take a look at it as soon as I am done with the debugging I am doing right now [10:37] lifeless: looking at r11324, you should call the result of removeSecurityProxy "naked_instance" or use rSP in place. [10:37] sure, I can do that [10:42] lifeless: why do you (think you) need the rSP calls? [10:42] because model objects hold security proxied references to other model objects [10:43] and having many little 'clear_method_x' named functions in the public interface would make me want to stab my eyeballs out :) [10:44] I think a few more iterations and we'll come up with some good way of managing these things that fits with interfaces and security proxies, but right now its very adhoc: these two branches clean it up a lot (IMNSHO), but also find the limits of the self same cleaned up protocol [10:49] lifeless: I have to admit I am on shaky ground here ... [10:50] ok [10:50] this is something new [10:51] I don't think that *anyone* can say 'I know all this stuff and its definitely good/bad/whatever' [10:51] there are acknowledged risks - and you can see curtis's comment: land it and we'll fix what breaks [10:51] these incremental changes are simply the first things that goes boom :) [10:56] lifeless: actually, my original question about the "why" was if you did see tests failing without the rSP in place. In your comment on the MP you say that you are not sure if they are needed at all. [10:56] henninge: oh, I mean sttructurally [10:56] like [10:57] can we find some (within scope of this branch) way to not need them [10:57] but I couldn't see one [10:57] ArchiveSet.new [10:57] references owner.archive [10:57] which populates the cache [10:57] and then takes an action that invalidates it IFF the result of the lookup was None [10:57] so the change to archive.py to invalidate has to work when the person object used for owner is security proxied [11:07] henninge: ? [11:09] lifeless: how about this: [11:10] do not do the blanked "removeSecurityProxy" in cachedpropety.py but rather do it in archive.py for the specific use case. [11:11] henninge: It looks to me like every use is likely to need it [11:11] AIUI the general case is that external references are proxied [11:12] lifeless: the use of these functions (not the decorator, of course) is quite limited. [11:12] henninge: of course, they are added today [11:14] lifeless: argh, missed that ... [11:14] so is_cached clearly has no downside for our code, doing rSP [11:14] likewise clear_* [11:14] cache_property *could* be misused [11:14] but I think it will stand out very clearly where its being used [11:14] so its ok [11:15] (IMHO) [11:15] lifeless: well, with these functions being new, we can asses each use of them more easily. [11:16] that too [11:16] we are not in danger to introduce a security hole where there was none before. [11:16] so in summary, I'd like to leave rSP in the readonly functions for sure [11:16] I'm open to moving rSP out of the cache_property call, but I think we'll end up being more annoyed than benefited :) [11:18] lifeless: I tink it is fine as long as it is clear that these are low-level functions that do not care about security. [11:18] I'll tweak the docstring on cache_property then [11:18] for the others, I think its not security related as such *anyhow* [11:19] because you can *at most* find out that an attribute exists, not get its value [11:19] lifeless: can you please create a seperate doctest for these functions instead of putting the tests in the docstrings? [11:19] We have been moving tests from docstrings into files. [11:19] well, doctests are pain anyhow [11:19] I guess that was one reason I thought this was (really) old code. [11:19] I followed the minimal path doing this though [11:19] so [11:20] how about we treat the tests - including the cachedproperty test - as techdebt, and I'll make a new branch to move it to lp.services, clean it up into proper unit tests and so on at the same time. [11:20] I can do that wednesdayish [11:21] lifeless: that's perfectly fine. [11:21] lifeless: another idea I had: It would be great if the functions could do the same name mangling as cachedproperty itself. [11:22] deal [11:22] cachedproperty doesn't name mangle :) [11:22] oh, I see the line you mean [11:22] uhm [11:22] yes, I can pull that out [11:22] problem is [11:22] we don't have the original fn to examine [11:22] so we could *claim* it was called 'foo' [11:22] and mangle that [11:22] but that will fail for all cases where an explicit cached attribute name was chosen [11:23] * henninge greps [11:23] so we'd need another parameter to say 'and don't mangle this' (or we'd probe twice) [11:23] line 76 [11:23] probing twice is unneeded overhead [11:24] lifeless: how about a named parameter? [11:24] and having two parameters for utility functions seems a bit ugh. I'd rather work on being able to do stuff with the actual descriptor [11:24] clear_property(owner, attr='archive') [11:24] clear_property(owner, name='_archive_cache') [11:24] ? [11:25] do you think it adds much ? [11:25] can always add it later if we decide we want it after all [11:25] I just think that the name mangling should be black boxed. [11:26] so, we couldn't really do this until I added these helper functions anyhow [11:26] I think you're right that it would be nicer to not require folk to thing about this [11:26] I have two concerns [11:26] yes, what I mean. Using cachedproperty is quite transparent atm [11:27] henninge: well, many users do cachedproperty('_name_to_use') already [11:27] anyhow concerns [11:27] setattr(thing, 'name', value) [11:27] == [11:27] cache_property(thing, 'name', value) [11:27] so its easy to translate in your head [11:28] cache_property(thing, attrname='name', value) # This is not legal python [11:28] so it would have to be [11:28] cache_property(think, value, attrname='name') [11:28] cache_property(think, value, attr='name') [11:29] which is the oppposite order and I worry that that is prone to confusion [11:29] I think I'd rather add in mangling versions later, if we need them [11:29] I understand. [11:29] Sure, was just my idea. [11:29] cache_attribute(thing, unmangled, value) [11:29] henninge: its a good idea [11:29] needed thought :) [11:30] Here's another one ;) [11:30] for a future branch. [11:30] It would be great to be able to prevent caching in the first place. [11:31] what do you think of waiting and seeing ? [11:31] instead of having to reset it right afterwards. [11:31] yes, just an idea, again ... [11:31] yeah, that gets back to the descriptor idea [11:32] 'permit a cache read but don't do a cache write' === noodles775 changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lifeless || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:35] henninge: Easy branch testing and fixing a recent JS breakage: https://code.edge.launchpad.net/~michael.nelson/launchpad/561586-qa-fail/+merge/32745 (63 lines) [11:37] lifeless: r=me ;-) === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:38] Hi noodles775! ;-) [11:38] Let me see if I can do this before lunch ... ;) [11:39] Thanks henninge [11:41] henninge: thank you! [11:41] lifeless: pleasure [11:42] noodles775: lint should complain about two extra blank lines at the end of the file ... [11:42] henninge: yes it did, and I fixed them, and forgot to push... [11:42] noodles775: np, r=me [11:42] Great, thanks henninge. === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === jelmer_ is now known as jelmer === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mrevell is now known as mrevell-lunch === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === mrevell-lunch is now known as mrevell [13:43] henninge, got a review for ya: https://code.edge.launchpad.net/~jtv/launchpad/recife-karmarecorder-fix/+merge/32756 === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:46] jtv: thanks ;) [13:47] henninge: you are ironing, I'm sure :) [13:47] Or whatever the verb for "being ironic" is [13:47] Now, I am done with my houshold chores. ;) [13:47] s/Now/No/ [13:48] jtv: XXX: Module docstring goes here. [13:48] henninge: whoa, thanks [13:49] henninge: do I see you're also waiting for a review? [13:49] jtv: yes, danilo hat offered to take it but if he is not up to it, I'd be happy if you could do it. [13:50] henninge: ah, that's the one you mentioned in the call… Yes, I think I can do that. [13:50] jtv: where is he anyway? [13:50] Just moving in mysterious ways, I'm sure. [13:53] jtv: comments on the tests wouldl be nice. ;) [13:54] henninge: can do [13:56] Done. [13:59] henninge: in your branch, in _makeTranslationsDict (diff line 8): if translations can be None, maybe it should just default to None—more conformant with factory practice. [14:01] jtv: I have copied that behavior from make TranslationMessage. [14:01] henninge: …where it has no business conforming with factory practice. Here, it does. :-) [14:01] henninge: then, I wouldn't document the method as "making sure" the data is in a particular format—it's a bit ambiguous as to what happens if it isn't. [14:02] jtv: is it not consistent with factory practice to make up a value of None is passed in? [14:03] henninge: it is consistent with factory practice to let a parameter default to None if you're going to accept a None and fill in a random value for testing. [14:03] So (self, translations=None) instead of (self, translations) [14:06] jtv: r=me btw [14:06] henninge: great, thanks === henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:06] let me look at that [14:10] jtv: ah, now I understand what you want. But this is an internal method, so the outward behavior is consistent as it is "makeSuggestion(translations=None)" [14:10] henninge: ok [14:10] But I can add the default paramter, np. [14:10] henninge: moving on to the second line… :-) [14:10] :-P [14:11] henninge: I wouldn't lead with "the right format" (IIRC the original code committed the same horror). What matters is that you want the dict format. Maybe an example would say it more clearly and in fewer characters: {0: 'foo'} [14:14] henninge, I have a short branch to add to your queue if that's okay. [14:19] henninge: I can't help but feel that trying to name all types that are a sequence but not a dict is a losing game… do tuples really matter there? What about ResultSets? Maybe you could rearrange the test: if translations is None / elif isinstance(translations, dict) / else [14:19] The "else" part would return dict(enumerate(translations)) [14:20] jtv, btw, are you reviewing henninge's branch? [14:20] danilos: he is, shouldn't he? [14:20] sure :) [14:20] danilos: yes… to keep him busy while we didn't hear from you :-) [14:20] It's eod here, but that also means it's too late to do other things. :) [14:20] gmb: that is ok [14:20] henninge, jtv was only supposed to be out by now :) [14:20] ;-) [14:21] danilos: he basically begged me to be allowed to review it. [14:21] I tell him "It's Danilo's!" [14:21] He says "Oh no, please, please, not him!" [14:21] henninge, hehe, don't worry, I don't mind too much :) [14:21] gee, how lucky (!) [14:22] henninge: there's no more need to do a separate setSequence after a makePOTMsgSet… just pass the sequence number to makePOTMsgSet. [14:22] henninge: oh, and when you use assert, there has to be an explanatory string. [14:23] gmb: bug 618606 ? [14:23] <_mup_> Bug #618606: Add bug_subscription_level to the BugSubscriptionModel [14:25] henninge, Sure. Sorry, missed your first ping. [14:26] gmb: would it be too much for this branch to move BugNotificationLevel to lp.registry.enum? [14:26] Why is it in registry in the first place? [14:27] henninge, structural subscriptions [14:27] sinzui: ok [14:27] henninge, I have no idea. I'd be +1 on moving it to lp.bugs.enum, given that it's bug-specific. Either that or changing it to SubscriptionNotificiationLevel [14:27] And leaving it in lp.registry. [14:27] henninge, SS are some what problematic because when fully implemented, they would know about bugs, specs, questions, and branches [14:28] sinzui: so it should be lp.coop/inter/shared.enum ... ;-) [14:28] whatever we decide on ... or have we already? [14:28] henninge, registry is exempt because every app depends on it...but that is also why it is problematic [14:29] henninge, I think the db scheme is the origin of the problem maybe [14:29] sinzui: Is there a problem with putting it in lp.registry.enum ? [14:30] No [14:30] cool ;) [14:30] gmb: ^ [14:30] Righto. [14:30] sinzui, Should it be renamed to something more generic so that it can be reused across different SS targets? [14:31] (e.g. SubscriptionNotificationLevel) [14:31] good idea [14:31] gmb: good idea [14:31] ;) [14:31] Cool [14:31] Will do. [14:32] I'll file a bug about renaming the attributes, since that's more work. [14:32] s/attributes/attributes and db fields/ [14:37] Ah. [14:38] henninge, sinzui: Maybe the rename doesn't make sense after all. There's also a BlueprintNotificationLevel. [14:38] So the rename makes sense if we're converging on one enum. [14:38] :( [14:38] But I think that's out of scope for this branch. [14:38] I'll do the move though and update the import sites. [14:40] gmb: We can revisit this in a few weeks when I need to look at ACLs for privacy. Maybe I can introduce a generic form for all artefacts [14:40] Right [14:49] gmb: r=me ;) === henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:49] henninge, Thanks. [14:55] henninge: did you get my comments about the docstring, and about rearranging the conditions in _makeTranslationsDict? [15:03] henninge: could you please re-run "make lint"? There's plenty of stuff in the diff that it would have complained about, so I think your previous run was incomplete somehow. [15:13] jtv: what was that about rearranging conditions? [15:14] henninge: Naming all types of sequence that might make up a non-dict translations list is probably a losing game: is it really necessary to accept tuples? What about ResultSets? If it's really needed, consider rearranging the conditions: "if translations is None / elif isinstance(translations, dict) / else" (where the "else" part returns dict(enumerate(translations))) [15:14] jtv: ah, god idea. [15:15] well I wouldn't go _that_ far… [15:17] henninge: I've got a ton more, a lot of it clearly not your doing. I'll paste in the review. [15:19] jtv: well, I was aware that I would not catch all iterables but that's what the assert was for. But this is just as good. [15:19] s/but/and/ [15:20] henninge: note that the assert would have needed an explanatory string btw. [15:21] And speaking of strings, I suppose if we treat everything else as an iterable, then someday we're going to pass a string instead of a list by mistake and get ['f', 'u', 'n', 'n', 'y'] results. [15:21] So that assertion may still be worthwhile. :-) [15:23] jtv: but then I'd be checking isinstance(translations, dict) twice ... :( [15:24] I think I'll return to the first structure and just add the comment to the assert. [15:24] henninge: well the assertion on dict type wouldn't be needed, would it? [15:24] which assertion were you referring to? [15:25] Sorry: I mean, you can assert on the right type—but in the clause for sequences. So assert isinstance(translations, (list, tuple)). [15:25] Then if we find that e.g. ResultSets need supporting, all we need to do is add that type to the assert. [15:26] (I think the error message would be clearer that way than with the current structure) [15:26] huh, so now we *are* back to the original structure? I am confused. [15:27] henninge: no, I'm referring to the original structure as the current one. [15:27] henninge: I can't see what you have locally. [15:27] you cannot? :-o [15:28] henninge: so I'm saying it would be good to do something like… if translations is None: return {0: makeString()} ; elif isinstance(translations, dict): return translations ; else: assert isinstance(translations, (tuple, list)); return dict(enumerate(translations)) [15:29] Hm, I don't see the big difference/advantage in that, though. [15:37] henninge: think about what happens if it goes wrong, e.g. because you're getting a ResultSet. [15:37] Think about the failure mode. [15:38] ok, we'd be getting something we don't expect. [15:38] jtv, henninge: is there any reason not to define one single interface (i.e. "only takes a dict") [15:38] jtv: but OTOH we are not checking that the elements of the list are really strings. [15:39] In the original code, you'd get a failure "this is not a dict." Which would be a fairly silly thing to expect it to be. In the rearranged version, you'd get a failure "this is neither a list or a tuple." That would instantly make you think: "clearly there are several things this is allowed to be, and clearly a ResultSet is a lot _like_ a list or tuple." [15:39] (I know I am jumping right into the middle of the discussion, but I'd go with simplest possible APIs) [15:39] danilos: well, lists are passed in many places and this seemed easier than checking all call sights. [15:39] site [15:39] sites [15:39] s [15:40] jtv: ok, let's end this here. ;-) [15:40] uh-oh… are we talking gunfight in the street? [15:43] jtv: nah, this is just taking longer then it is worth. Any idea for a good failure message for the assertion? [15:43] "Translations must be in a dict, list or tuple." [15:43] henninge: er... "Expecting either a dict or a sequence"? [15:44] (so as to document intent, not what the code does) [15:45] ok [15:45] I pasted the rest of my notes in the MP. [15:46] jtv: here is the lint output before any changes, btw. [15:46] http://paste.ubuntu.com/478885/ [15:46] henninge: maybe some messages are being suppressed then. [15:47] I think the linters expectations on blank lines need to be fixed and I don't know what form to raise exceptions in ... [15:47] jtv: thanks [15:47] for the review [15:47] have not seen it yet, though :-P [15:47] henninge: it's rather a lot, and mostly probably not about stuff you did so feel free to push back a bit === salgado is now known as salgado-lunch === Ursinha is now known as Ursinha-brb === matsubara is now known as matsubara-lunch === Ursinha-brb is now known as Ursinha === salgado-lunch is now known as salgado [18:12] EdwinGrubbs, I shot myself in the foot with a bad branch and I used my own project for the test. [18:12] EdwinGrubbs, Can you read through this bug and then talk to me about whether my proposed fix will help undo future damge: https://bugs.edge.launchpad.net/launchpad-registry/+bug/522599 === matsubara-lunch is now known as matsubara [18:21] sinzui: I'm looking at it now. [18:30] EdwinGrubbs, I am ready to mumble and explain how I screwed up [18:44] EdwinGrubbs :Store.of(bugtask).remove(bugtask.conjoined_master) === EdwinGrubbs is now known as Edwin-lunch [19:16] sinzui: hiya [19:19] hi lifeless [19:42] anybody willing to review a small branch for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-no-external-redirect/+merge/32623 [19:45] salgado, yes [19:46] sinzui, yay, thanks! [19:46] salgado, I have a 25 line branch I wanted reviewed. the mp will show up in a few minutes. Can you take a look at it when it does. [19:47] sinzui, sure [19:51] salgado, Is your branch intended to ensure any reuse of lp logic does not take the user to lp.net? [19:52] sinzui, yes, but it's a nice-to-have safety net anyway. we only had to remove that in Launchpad because we have multiple hosts we can redirect people to [19:53] salgado, I always think links to attachments/changelogs as redirects to the librarian. Will these links still work? [19:55] sinzui, in vostok we probably won't have attachments, but we'll certainly store other things in the librarian, so we might have to change that later to allow redirects to the librarian as well [19:55] but we can worry about that when we need it [19:56] Will API calls work? Is API not a part of vostock [19:56] That may be moot. We never redirect to API [19:56] this shouldn't affect the API [19:59] salgado, r=me. I have no other questions [19:59] thanks sinzui. where's yours? [19:59] salgado, https://code.edge.launchpad.net/~sinzui/launchpad/delete-conjoined-bugtask-1/+merge/32796 [20:01] salgado, I should note that Edwin-lunch fixed by project data. But he too saw a bugtask targeted to trunk with no project bug task. [20:07] sinzui, r=me [20:07] thanks salgado [20:31] sinzui, since you just got a review, would you please do a (small) review for me? https://code.edge.launchpad.net/~rockstar/launchpad/no-recipes-on-private-branches/+merge/32799 [20:31] I can start in a few minutes [20:43] rockstar, I had a few questions. Your branch is really fine as it is, but I want to understand if there is something I am missing. [20:43] sinzui, cool. [20:45] sinzui, so, I haven't used the formatters in the past because we want to use javascript on this feature (_before_ we call the feature done) and the formatters don't yet let you specify an id. [20:46] good reason [20:47] sinzui, I'll use the with person_logged_in === matsubara is now known as matsubara-afk [20:47] Thanks for the explanation. I will update the review with my new insight. [20:50] rockstar, r=me [20:50] sinzui, thanks. === Edwin-lunch is now known as EdwinGrubbs === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === salgado is now known as salgado-afk