| jam | fwereade: there you are | 09:02 |
|---|---|---|
| jam | I hereby revoke your ability to submit any more patches :) | 09:02 |
| fwereade | jam, hey dude, tyvm for all the review | 09:02 |
| fwereade | jam, haha | 09:03 |
| fwereade | jam, that's what happens when my family go away for a couple of days and there's noone on irc :) | 09:03 |
| dimitern | fwereade: you're on fire on friday :) | 09:04 |
| fwereade | dimitern, yeah, 'twas a great day | 09:04 |
| fwereade | jam, I was thinking of adding explicit tests for the charm test helpers to charm_test.go, would that be ok with you? | 09:05 |
| jam | fwereade: I've been told you don't write tests for test helpers, but *I'm* personally a fan of it. | 09:06 |
| jam | (Why is this test failing? Oh the test infrastructure is wrong) | 09:06 |
| fwereade | jam, yeah, me too, it's just been 4 years since I've been in an environment where someone else agrees with me on this | 09:06 |
| dimitern | jam: are we the only ones today for standup? | 10:26 |
| jam | dimitern: technically, but mgz is sitting on mumble | 10:33 |
| fwereade | dimitern, fwiw jam's observations on the txns are good ones, and I'm still trying to figure out the Right Thing to do in those cases, but I'm definitely not quite doing it right now | 11:47 |
| fwereade | dimitern, jam: the big question for me is what I can safely assume about ops that aren't constructed in plain view... *at the moment* we can simplify those my assuming how the op-creation methods work | 11:48 |
| fwereade | dimitern, jam: and *maybe* we should | 11:48 |
| jam | fwereade: I did a lot of reviewing, can you link for context? | 11:49 |
| fwereade | jam, https://codereview.appspot.com/8126044/diff/1/state/machine.go#newcode310 | 11:50 |
| fwereade | jam, https://codereview.appspot.com/8126044/diff/1/state/machine.go#newcode447 | 11:50 |
| fwereade | jam, I have ideas floating around about a layer that lets us work with ops without having to think so much | 11:52 |
| jam | fwereade: so if you have a case where retrying will succeed when it would otherwise fail, I'm happy to have the retries | 11:53 |
| jam | though a more consistent "try twice" or "try three times" seems useful | 11:53 |
| fwereade | jam, "try twice" was deliberate, and I think it's correct but its form may be unhelpful -- the idea there is that we use the exact same gating checks across the board, but an aborted txn should guarantee that one of those will fail; we should therefore always return before attempting to execute a second txn | 11:56 |
| fwereade | jam, deserves a comment, I think | 11:57 |
| fwereade | jam, the reason for the "try three times" was because I expect lifecycle switches to be icky and demand retries, but neglected to observe that this one's simpler than I assumed | 11:58 |
| fwereade | jam, I would be most keen to go for a try-twice approach there for the above reasons | 11:58 |
| fwereade | jam, I'd like to roll up the duplicated bits of the txn-futzing code into something a little smarter, but I don't think I can justify it atm because it'd be at least half fishing-trip | 12:02 |
| fwereade | mramm, hey, I think it's just me again -- do we need to meet? I have a pile of good reviews and I'll be landing them for the rest of the day I think, not much more to tell | 13:57 |
| === jcastro_ is now known as jcastro | ||
| mramm | fwereade: if I have any questions about the board status, I'll let you know -- but no we don't need a meeting. | 13:58 |
| fwereade | mramm, cool | 13:58 |
| gary_poster | sorry for basic go question, but what is diff between []params.Port(nil) and []params.Port{} ? Test expects first and obtains second | 14:50 |
| gary_poster | pointer to doc would be aok :-) | 14:52 |
| dimitern | gary_poster: I think the first one is just a nil slice, while the second one is an empty one. Both have len() == 0, but the first cannot be modified. The second is equivalent to make([]params.Port, 0) | 15:00 |
| gary_poster | ah! thanks dimitern. I'll see if I can switch one to the other... | 15:02 |
| dimitern | gary_poster: actually, both can be modified ok it seems: http://play.golang.org/p/Mr2Y49I2Q2 | 15:06 |
| gary_poster | dimitern, oh, weird. So...should DeepEquals find them equivalent? | 15:06 |
| gary_poster | append can return back a new instance IIRC | 15:07 |
| gary_poster | so you might still be right about initialization diffeerences | 15:07 |
| dimitern | gary_poster: yeah, you're right - append creates a new slice | 15:08 |
| dimitern | gary_poster: deepequals should work I think | 15:08 |
| dimitern | gary_poster: they're of the same type | 15:08 |
| gary_poster | dimitern, http://pastebin.ubuntu.com/5667597/ ISTM that DeepEquals does not behave that way now? Or am I missing something? | 15:09 |
| dimitern | gary_poster: https://groups.google.com/forum/?fromgroups=#!topic/golang-nuts/vNodYyoUu9Q - there are no docs about it, but there are some ML questions about it | 15:11 |
| dimitern | gary_poster: so "the value of an uninitialized slice is nil" | 15:12 |
| fwereade | gary_poster, I suggest using HasLen for slices that might be nil or empty | 15:13 |
| fwereade | gary_poster, assuming you're testing | 15:13 |
| fwereade | gary_poster, also works on maps | 15:13 |
| dimitern | gary_poster: HasLen 0 works on both maps and slices, even if nil, so should work | 15:14 |
| gary_poster | fwereade, ack, ty. So given context (http://pastebin.ubuntu.com/5667597/) of testing larger struct, would you suggest refactoring that DeepEquals assertion to stepping over each attr individually? | 15:14 |
| dimitern | gary_poster: the issue for me here is why []T(nil) is returned when it need to return []T{} probably? | 15:14 |
| gary_poster | yeah, I was going to try and address that | 15:15 |
| fwereade | gary_poster, doing every field seems a bit nasty | 15:15 |
| dimitern | try c.Assert(gotEntities[num].Port, DeepEquals, ent.Port) | 15:16 |
| fwereade | gary_poster, I'd probably prefer to fix either the return value or what the test checks for | 15:16 |
| gary_poster | fwereade, I thought so too. Cool, I'll pursue that. thank you again, fwereade and dimitern | 15:16 |
| fwereade | gary_poster, cheers | 15:16 |
| dimitern | if it fails, then I suppose digging into reflect.DeepEqual to see what might be the issue | 15:16 |
| fwereade | dimitern, AIUI nil is not the same as an empty slice, it just acts the same in most situations | 15:17 |
| fwereade | dimitern, DeepEquals is not one of those situations | 15:17 |
| fwereade | dimitern, consider maps -- nil maps act like empty maps in many situations, but not all | 15:18 |
| fwereade | dimitern, maps are a bit worse off because there's no equivalent of append for a map, so the difference is a little more obvious | 15:18 |
| dimitern | fwereade: yeah, I was thinking something in those lines | 15:22 |
| dimitern | fwereade: and the docs are sparse at places ("if it's not in the docs it's not allowed " etc.) | 15:22 |
| fwereade | gn all, hope to pop on and do a few of other people's reviews a bit later | 16:15 |
| dimitern | fwereade: have a good one | 16:15 |
| === Makyo is now known as Makyo|out | ||
| === BradCrittenden is now known as bac__ | ||
| === bac__ is now known as bac | ||
| === Makyo|out is now known as Makyo | ||
| thumper | morning | 19:57 |
| gary_poster | hi | 20:11 |
| gary_poster | I'd really appreciate two reviews of this large but almost-but-not-entirely mechanical branch (circular imports): https://codereview.appspot.com/8086045/ | 20:13 |
| thumper | hi gary_poster | 20:37 |
| gary_poster | hey thumper :-) | 20:37 |
| thumper | gary_poster: why the new package for params? | 20:39 |
| thumper | gary_poster: instead of using "series" for series and "name" for the service name with comments for "precise" and "wordpress", why not just structure the test so the results show "precise" and "wordpress" ? | 20:42 |
| gary_poster | thumper, I did not add the params package, I only added the values to the params package. params package is supposed to be a place to put structs that are used both by state and by api | 20:43 |
| thumper | ok | 20:44 |
| gary_poster | the two are not to mix, AIUI; and certainly when they do you get circular imports | 20:44 |
| thumper | I bet most of those changes were mind-numbingly boring | 20:44 |
| gary_poster | why yes! :-) | 20:44 |
| gary_poster | thunper, +1 on changing the examples to the comments. | 20:45 |
| gary_poster | thumper, even | 20:45 |
| * thumper writes it on the review | 20:46 | |
| gary_poster | ty | 20:46 |
| * thumper looks at the kanban board for something to do | 20:50 | |
| m_3 | thumper: I don't have a leankit acct... would you mind linking lp:~mark-mims/+junk/pyju-packaging-with-alternatives to the packaging item | 21:27 |
| m_3 | thumper: it's working, but martin can review/cleanup when he gets back from vaca | 21:28 |
| thumper | m_3: sure... | 21:28 |
| m_3 | thumper: thanks! | 21:28 |
| m_3 | thumper: go packaging should be able to do something similar | 21:29 |
| * thumper nods | 21:29 | |
| m_3 | awesome... got an instance running with both 0.6 and 1.9.13 installed from packages | 23:25 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!