/srv/irclogs.ubuntu.com/2010/08/16/#launchpad-reviews.txt

lifelesshmm, I wants  areviewer01:11
lifeless https://code.launchpad.net/~lifeless/launchpad/cachedproperty/+merge/3272801:11
lifelessanyone up for it ?01:11
lifelessmwhudson: not that I'm stalking you or anything .... pretty please?01:11
* mwhudson looks01:11
mwhudsonlifeless: i guess you grepped for existing __storm_invalidate__ methods so i don't have to? :)01:16
lifelessyeah01:16
lifelessin devel as of latest commit a couple of hours ago01:16
lifelessof course, folk can still add them01:16
lifelessso it needs a psg update, mail to the list, various repeats etc etc01:17
mwhudsonlifeless: approved01:20
lifeless\o/01:21
lifelessmwhudson: click the other button too01:23
lifeless(please)01:23
mwhudsonlifeless: done01:23
mwhudsonlifeless: can't you do that though?01:23
lifelessmwhudson: I can, but its a pita :)01:24
mwhudsonok01:24
lifelessso I try to do it for other people when I review their branches01:24
lifelessso that when they are told approved, they just need to do 'ec2 land'...01:24
lifelessrather than reopen the web page, etc etc01:24
lifelessanyhow thanks heaps for the review01:25
lifelesssorry if that last bit came across grumpy01:25
lifelessits just high friction01:25
mwhudsonyeah, it was a slip on my part i agree01:27
mwhudsonthanks for the prod01:27
lifelesshah, so the SQLBase super() call is bogus01:40
lifelessbecause the base class doesn't have one, so I'll nuke that :)01:40
lifeless(the test suite does blow up, I just hadn't run model class tests ;P)01:41
lifelessmwhudson: around? I'm about to pop in a fix or two to that branch (nonobvious in foresight, clear in hindsight)02:51
lifelessmwhudson: rev 11352 and 11353 on lp:~lifeless/launchpad/cachedproperty03:00
mwhudsonlifeless: looking03:02
lifelessthanks!03:03
mwhudsonlifeless: looks fine03:06
lifelessthanks03:06
mwhudsonthe need for 11353 is a bit depressing03:06
lifelessyes03:06
mwhudsonhm03:06
mwhudsonoh nm03:06
lifelessOTOH its good that folk are *already* using cachedproperty :P03:06
mwhudsonwas thinking that maybe you could write this as self.cachedprop.cache(value)03:06
mwhudsonbut obviously not03:06
mwhudsonbecause of what self.cachedprop does :)03:07
lifelessyes03:08
lifelessit would be lovely03:08
lifelessbut sadly, DENIED03:08
lifelessit might be an idea to make _cachedproperties a dict03:08
lifelessand then clear_properties can just replace the dict03:08
lifelessbut since its now nicely abstracted ('nicely') we can do that later.03:09
mwhudsonwell, i guess you can do self.__class__.cachedprop03:09
lifelessI don't think so03:10
lifelesswhen the decorator is invoked you get a bare function03:10
lifelessno im_class attribute03:10
lifelessand definitely no instances (instances being what have __class__)03:10
lifelessoh, I see03:10
lifelessuhm, yes, we could make it into a obj with methods that you access via self.__class__.publicname03:11
lifelessself.__class__.publicname.clear(self)03:11
lifelessit a tad ugly03:11
mwhudsonyeah, i think it's a bit ugly03:12
mwhudsonso let's stick with what you have for now03:13
=== 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
henningeBring on your branches!08:49
=== 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
StevenKhenninge: Hi! :-)10:13
lifelesshenninge: hi, I've made some changes to a branch sinzui approved, how do you feel about an incremental review ?10:19
henningelifeless: Unless it's urgent I'd like you to take it back to sinzui, actually .. ;-)10:20
lifelesshenninge: well, given ec2 takes 4 hours to land stuff, I was hoping to throw it in overnight :)10:21
henningeah right, forgot were you are ... ;)10:21
lifeless:P10:21
henningelifeless: put it on the queue10:22
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/registry/+merge/3206710:23
=== 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
lifelesshenninge: there is one commit in particular I want to discuss - http://bazaar.launchpad.net/~lifeless/launchpad/registry/revision/1132410:23
lifelesswhich is not domain specific10:23
lifelesswhen you're done with mr K10:23
henningeok ;)10:23
StevenK Mine is only a widdle branch10:24
henningeStevenK: Hi! Looking ;10:24
henningeStevenK: 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
henningeStevenK: other than that, r=me ;-)10:27
lifelesswe did, imports are now like any other list.10:27
lifelesswell, nearly - still alpha-sorted please :)10:28
lifelesshenninge: so, my branch looks big, but all the bulky stuff is already reviewed10:29
=== 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
* henninge looks10:30
lifelessI've explained in the review page the incremental stuff I would love to have more eyeballs on10:30
henningedanilos: the branch is ready for review btw.10:31
daniloshenninge, cool, thanks, I'll take a look at it as soon as I am done with the debugging I am doing right now10:31
henningelifeless: looking at r11324, you should call the result of removeSecurityProxy "naked_instance" or use rSP in place.10:37
lifelesssure, I can do that10:37
henningelifeless: why do you (think you) need the rSP calls?10:42
lifelessbecause model objects hold security proxied references to other model objects10:42
lifelessand having many little 'clear_method_x' named functions in the public interface would make me want to stab my eyeballs out :)10:43
lifelessI 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 protocol10:44
henningelifeless: I have to admit I am on shaky ground here ...10:49
lifelessok10:50
lifelessthis is something new10:50
lifelessI don't think that *anyone* can say 'I know all this stuff and its definitely good/bad/whatever'10:51
lifelessthere are acknowledged risks - and you can see curtis's comment: land it and we'll fix what breaks10:51
lifelessthese incremental changes are simply the first things that goes boom :)10:51
henningelifeless: 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
lifelesshenninge: oh, I mean sttructurally10:56
lifelesslike10:56
lifelesscan we find some (within scope of this branch) way to not need them10:57
lifelessbut I couldn't see one10:57
lifelessArchiveSet.new10:57
lifelessreferences owner.archive10:57
lifelesswhich populates the cache10:57
lifelessand then takes an action that invalidates it IFF the result of the lookup was None10:57
lifelessso the change to archive.py to invalidate has to work when the person object used for owner is security proxied10:57
lifelesshenninge: ?11:07
henningelifeless: how about this:11:09
henningedo not do the blanked "removeSecurityProxy" in cachedpropety.py but rather do it in archive.py for the specific use case.11:10
lifelesshenninge: It looks to me like every use is likely to need it11:11
lifelessAIUI the general case is that external references are proxied11:11
henningelifeless: the use of these functions (not the decorator, of course) is quite limited.11:12
lifelesshenninge: of course, they are added today11:12
henningelifeless: argh, missed that ...11:14
lifelessso is_cached clearly has no downside for our code, doing rSP11:14
lifelesslikewise clear_*11:14
lifelesscache_property *could* be misused11:14
lifelessbut I think it will stand out very clearly where its being used11:14
lifelessso its ok11:14
lifeless(IMHO)11:15
henningelifeless: well, with these functions being new, we can asses each use of them more easily.11:15
lifelessthat too11:16
henningewe are not in danger to introduce a security hole where there was none before.11:16
lifelessso in summary, I'd like to leave rSP in the readonly functions for sure11:16
lifelessI'm open to moving rSP out of the cache_property call, but I think we'll end up being more annoyed than benefited :)11:16
henningelifeless: 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
lifelessI'll tweak the docstring on cache_property then11:18
lifelessfor the others, I think its not security related as such *anyhow*11:18
lifelessbecause you can *at most* find out that an attribute exists, not get its value11:19
henningelifeless: can you please create a seperate doctest for these functions instead of putting the tests in the docstrings?11:19
henningeWe have been moving tests from docstrings into files.11:19
lifelesswell, doctests are pain anyhow11:19
henningeI guess that was one reason I thought this was (really) old code.11:19
lifelessI followed the minimal path doing this though11:19
lifelessso11:19
lifelesshow 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
lifelessI can do that wednesdayish11:20
henningelifeless: that's perfectly fine.11:21
henningelifeless: another idea I had: It would be great if the functions could do the same name mangling as cachedproperty itself.11:21
lifelessdeal11:22
lifelesscachedproperty doesn't name mangle :)11:22
lifelessoh, I see the line you mean11:22
lifelessuhm11:22
lifelessyes, I can pull that out11:22
lifelessproblem is11:22
lifelesswe don't have the original fn to examine11:22
lifelessso we could *claim* it was called 'foo'11:22
lifelessand mangle that11:22
lifelessbut that will fail for all cases where an explicit cached attribute name was chosen11:22
* henninge greps11:23
lifelessso we'd need another parameter to say 'and don't mangle this' (or we'd probe twice)11:23
lifelessline 7611:23
lifelessprobing twice is unneeded overhead11:23
henningelifeless: how about a named parameter?11:24
lifelessand having two parameters for utility functions seems a bit ugh. I'd rather work on being able to do stuff with the actual descriptor11:24
henningeclear_property(owner, attr='archive')11:24
henningeclear_property(owner, name='_archive_cache')11:24
henninge?11:24
lifelessdo you think it adds much ?11:25
lifelesscan always add it later if we decide we want it after all11:25
henningeI just think that the name mangling should be black boxed.11:25
lifelessso, we couldn't really do this until I added these helper functions anyhow11:26
lifelessI think you're right that it would be nicer to not require folk to thing about this11:26
lifelessI have two concerns11:26
henningeyes, what I mean. Using cachedproperty is quite transparent atm11:26
lifelesshenninge: well, many users do cachedproperty('_name_to_use') already11:27
lifelessanyhow concerns11:27
lifelesssetattr(thing, 'name', value)11:27
lifeless==11:27
lifelesscache_property(thing, 'name', value)11:27
lifelessso its easy to translate in your head11:27
lifelesscache_property(thing, attrname='name', value) # This is not legal python11:28
lifelessso it would have to be11:28
lifelesscache_property(think, value, attrname='name')11:28
lifelesscache_property(think, value, attr='name')11:28
lifelesswhich is the oppposite order and I worry that that is prone to confusion11:29
lifelessI think I'd rather add in mangling versions later, if we need them11:29
henningeI understand.11:29
henningeSure, was just my idea.11:29
lifelesscache_attribute(thing, unmangled, value)11:29
lifelesshenninge: its a good idea11:29
lifelessneeded thought :)11:29
henningeHere's another one ;)11:30
henningefor a future branch.11:30
henningeIt would be great to be able to prevent caching in the first place.11:30
lifelesswhat do you think of waiting and seeing ?11:31
henningeinstead of having to reset it right afterwards.11:31
henningeyes, just an idea, again ...11:31
lifelessyeah, that gets back to the descriptor idea11:31
lifeless'permit a cache read but don't do a cache write'11:32
=== 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
noodles775henninge: 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:35
henningelifeless: r=me ;-)11:37
=== 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
henningeHi noodles775! ;-)11:38
henningeLet me see if I can do this before lunch ... ;)11:38
noodles775Thanks henninge11:39
lifelesshenninge: thank you!11:41
henningelifeless: pleasure11:41
henningenoodles775: lint should complain about two extra blank lines at the end of the file ...11:42
noodles775henninge: yes it did, and I fixed them, and forgot to push...11:42
henningenoodles775: np, r=me11:42
noodles775Great, thanks henninge.11:42
=== 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
jtvhenninge, got a review for ya: https://code.edge.launchpad.net/~jtv/launchpad/recife-karmarecorder-fix/+merge/3275613:43
=== 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
henningejtv: thanks ;)13:46
jtvhenninge: you are ironing, I'm sure :)13:47
jtvOr whatever the verb for "being ironic" is13:47
henningeNow, I am done with my houshold chores. ;)13:47
henninges/Now/No/13:47
henningejtv: XXX: Module docstring goes here.13:48
jtvhenninge: whoa, thanks13:48
jtvhenninge: do I see you're also waiting for a review?13:49
henningejtv: 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:49
jtvhenninge: ah, that's the one you mentioned in the call…  Yes, I think I can do that.13:50
henningejtv: where is he anyway?13:50
jtvJust moving in mysterious ways, I'm sure.13:50
henningejtv: comments on the tests wouldl be nice. ;)13:53
jtvhenninge: can do13:54
jtvDone.13:56
jtvhenninge: 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.13:59
henningejtv: I have copied that behavior from make TranslationMessage.14:01
jtvhenninge: …where it has no business conforming with factory practice.  Here, it does.  :-)14:01
jtvhenninge: 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:01
henningejtv: is it not consistent with factory practice to make up a value of None is passed in?14:02
jtvhenninge: 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
jtvSo (self, translations=None) instead of (self, translations)14:03
henningejtv: r=me btw14:06
jtvhenninge: great, thanks14:06
=== 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
henningelet me look at that14:06
henningejtv: 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
jtvhenninge: ok14:10
henningeBut I can add the default paramter, np.14:10
jtvhenninge: moving on to the second line…  :-)14:10
henninge:-P14:10
jtvhenninge: 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:11
gmbhenninge, I have a short branch to add to your queue if that's okay.14:14
jtvhenninge: 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) / else14:19
jtvThe "else" part would return dict(enumerate(translations))14:19
danilosjtv, btw, are you reviewing henninge's branch?14:20
henningedanilos: he is, shouldn't he?14:20
danilossure :)14:20
jtvdanilos: yes… to keep him busy while we didn't hear from you :-)14:20
jtvIt's eod here, but that also means it's too late to do other things.  :)14:20
henningegmb: that is ok14:20
daniloshenninge, jtv was only supposed to be out by now :)14:20
henninge;-)14:20
henningedanilos: he basically begged me to be allowed to review it.14:21
henningeI tell him "It's Danilo's!"14:21
henningeHe says "Oh no, please, please, not him!"14:21
daniloshenninge, hehe, don't worry, I don't mind too much :)14:21
jtvgee, how lucky (!)14:21
jtvhenninge: there's no more need to do a separate setSequence after a makePOTMsgSet… just pass the sequence number to makePOTMsgSet.14:22
jtvhenninge: oh, and when you use assert, there has to be an explanatory string.14:22
henningegmb: bug 618606 ?14:23
_mup_Bug #618606: Add bug_subscription_level to the BugSubscriptionModel <story-better-bug-notification> <Launchpad Bugs:In Progress by gmb> <https://launchpad.net/bugs/618606>14:23
gmbhenninge, Sure. Sorry, missed your first ping.14:25
henningegmb: would it be too much for this branch to move BugNotificationLevel to lp.registry.enum?14:26
henningeWhy is it in registry in the first place?14:26
sinzuihenninge, structural subscriptions14:27
henningesinzui: ok14:27
gmbhenninge, 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 SubscriptionNotificiationLevel14:27
gmbAnd leaving it in lp.registry.14:27
sinzuihenninge, SS are some what problematic because when fully implemented, they would know about bugs, specs, questions, and branches14:27
henningesinzui: so it should be lp.coop/inter/shared.enum ... ;-)14:28
henningewhatever we decide on ... or have we already?14:28
sinzuihenninge, registry is exempt because every app depends on it...but that is also why it is problematic14:28
sinzuihenninge, I think the db scheme is the origin of the problem maybe14:29
henningesinzui: Is there a problem with putting it in lp.registry.enum ?14:29
sinzuiNo14:30
henningecool ;)14:30
henningegmb:  ^14:30
gmbRighto.14:30
gmbsinzui, Should it be renamed to something more generic so that it can be reused across different SS targets?14:30
gmb(e.g. SubscriptionNotificationLevel)14:31
sinzuigood idea14:31
henningegmb: good idea14:31
henninge;)14:31
gmbCool14:31
gmbWill do.14:31
gmbI'll file a bug about renaming the attributes, since that's more work.14:32
gmbs/attributes/attributes and db fields/14:32
gmbAh.14:37
gmbhenninge, sinzui: Maybe the rename doesn't make sense after all. There's also a BlueprintNotificationLevel.14:38
gmbSo the rename makes sense if we're converging on one enum.14:38
sinzui:(14:38
gmbBut I think that's out of scope for this branch.14:38
gmbI'll do the move though and update the import sites.14:38
sinzuigmb: 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 artefacts14:40
gmbRight14:40
henningegmb: r=me ;)14:49
=== 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
gmbhenninge, Thanks.14:49
jtvhenninge: did you get my comments about the docstring, and about rearranging the conditions in _makeTranslationsDict?14:55
jtvhenninge: 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:03
henningejtv: what was that about rearranging conditions?15:13
jtvhenninge: 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
henningejtv: ah, god idea.15:14
jtvwell I wouldn't go _that_ far…15:15
jtvhenninge: I've got a ton more, a lot of it clearly not your doing.  I'll paste in the review.15:17
henningejtv: 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
henninges/but/and/15:19
jtvhenninge: note that the assert would have needed an explanatory string btw.15:20
jtvAnd 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
jtvSo that assertion may still be worthwhile.  :-)15:21
henningejtv: but then I'd be checking isinstance(translations, dict) twice ... :(15:23
henningeI think I'll return to the first structure and just add the comment to the assert.15:24
jtvhenninge: well the assertion on dict type wouldn't be needed, would it?15:24
henningewhich assertion were you referring to?15:24
jtvSorry: I mean, you can assert on the right type—but in the clause for sequences.  So assert isinstance(translations, (list, tuple)).15:25
jtvThen if we find that e.g. ResultSets need supporting, all we need to do is add that type to the assert.15:25
jtv(I think the error message would be clearer that way than with the current structure)15:26
henningehuh, so now we *are* back to the original structure? I am confused.15:26
jtvhenninge: no, I'm referring to the original structure as the current one.15:27
jtvhenninge: I can't see what you have locally.15:27
henningeyou cannot? :-o15:27
jtvhenninge: 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:28
henningeHm, I don't see the big difference/advantage in that, though.15:29
jtvhenninge: think about what happens if it goes wrong, e.g. because you're getting a ResultSet.15:37
jtvThink about the failure mode.15:37
henningeok, we'd be getting something we don't expect.15:38
danilosjtv, henninge: is there any reason not to define one single interface (i.e. "only takes a dict")15:38
henningejtv: but OTOH we are not checking that the elements of the list are really strings.15:38
jtvIn 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
danilos(I know I am jumping right into the middle of the discussion, but I'd go with simplest possible APIs)15:39
henningedanilos: well, lists are passed in many places and this seemed easier than checking all call sights.15:39
henningesite15:39
henningesites15:39
jtvs15:39
henningejtv: ok, let's end this here. ;-)15:40
jtvuh-oh… are we talking gunfight in the street?15:40
henningejtv: nah, this is just taking longer then it is worth. Any idea for a good failure message for the assertion?15:43
henninge"Translations must be in a dict, list or tuple."15:43
jtvhenninge: er... "Expecting either a dict or a sequence"?15:43
jtv(so as to document intent, not what the code does)15:44
henningeok15:45
jtvI pasted the rest of my notes in the MP.15:45
henningejtv: here is the lint output before any changes, btw.15:46
henningehttp://paste.ubuntu.com/478885/15:46
jtvhenninge: maybe some messages are being suppressed then.15:46
henningeI think the linters expectations on blank lines need to be fixed and I don't know what form to raise exceptions in ...15:47
henningejtv: thanks15:47
henningefor the review15:47
henningehave not seen it yet, though :-P15:47
jtvhenninge: it's rather a lot, and mostly probably not about stuff you did so feel free to push back a bit15:47
=== 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
sinzuiEdwinGrubbs, I shot myself in the foot with a bad branch and I used my own project for the test.18:12
sinzuiEdwinGrubbs, 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/52259918:12
=== matsubara-lunch is now known as matsubara
EdwinGrubbssinzui: I'm looking at it now.18:21
sinzuiEdwinGrubbs, I am ready to mumble and explain how I screwed up18:30
sinzuiEdwinGrubbs :Store.of(bugtask).remove(bugtask.conjoined_master)18:44
=== EdwinGrubbs is now known as Edwin-lunch
lifelesssinzui: hiya19:16
sinzuihi lifeless19:19
salgadoanybody willing to review a small branch for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-no-external-redirect/+merge/3262319:42
sinzuisalgado, yes19:45
salgadosinzui, yay, thanks!19:46
sinzuisalgado, 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:46
salgadosinzui, sure19:47
sinzuisalgado, Is your branch intended to ensure any reuse of lp logic does not take the user to lp.net?19:51
salgadosinzui, 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 to19:52
sinzuisalgado,  I always think links to attachments/changelogs as redirects to the librarian. Will these links still work?19:53
salgadosinzui, 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 well19:55
salgadobut we can worry about that when we need it19:55
sinzuiWill API calls work? Is API not a part of vostock19:56
sinzuiThat may be moot. We never redirect to API19:56
salgadothis shouldn't affect the API19:56
sinzuisalgado, r=me. I have no other questions19:59
salgadothanks sinzui.  where's yours?19:59
sinzuisalgado, https://code.edge.launchpad.net/~sinzui/launchpad/delete-conjoined-bugtask-1/+merge/3279619:59
sinzuisalgado, 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:01
salgadosinzui, r=me20:07
sinzuithanks salgado20:07
rockstarsinzui, 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/3279920:31
sinzuiI can start in a few minutes20:31
sinzuirockstar, 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
rockstarsinzui, cool.20:43
rockstarsinzui, 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:45
sinzuigood reason20:46
rockstarsinzui, I'll use the with person_logged_in20:47
=== matsubara is now known as matsubara-afk
sinzuiThanks for the explanation. I will update the review with my new insight.20:47
sinzuirockstar, r=me20:50
rockstarsinzui, thanks.20:50
=== 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

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!