<div dir="ltr">I have been in situations in past jobs where adding one field to one logical entity required changing the signature of a half dozen structures & functions that were just copying data back and forth for little to no benefit. This is a pain in the ass, error prone, and should be avoided.<div><br></div><div>One of the things I like best about Go is that structs can be "just data". How you interact with the data can be different from layer to layer, but there's no reason why we need different structs with the same layout in different packages just for the illusion of separation of concerns. If you modify one, you have to modify all of them, thus you might as well just have one they all use. At least then it's clear which places in the code depend on this logical data structure, rather than having to try to grep for everywhere we copy the data from one structure to another. This is why having the api/params leaf package is a good idea - it's just data. Any package can use those structs to layout their data in a way that other packages can understand.</div><div><br></div><div>I don't see how having different identical structs that are updated simultaneously in any way prevents any problems with compatibility.</div><div><br></div><div>If they're different structs, they cannot possibly actually represent the same data on both sides of the wire, can they? One or both of the structs must have information that is not present on the wire and that information should be in a different type (which may embed the wire-data-struct). </div><div><br></div><div>Maybe I'm missing something from the proposal, but it seems like this just adds busywork without actually solving any problems that weren't caused by incorrect use of the code in the first place.</div><div><br></div><div>Separation of logic is absolutely a good thing. Separation of data is not nearly so useful.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 11, 2014 at 3:46 AM, roger peppe <span dir="ltr"><<a href="mailto:roger.peppe@canonical.com" target="_blank">roger.peppe@canonical.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 11 September 2014 06:40, John Meinel <<a href="mailto:john@arbash-meinel.com">john@arbash-meinel.com</a>> wrote:<br> >> ...<br> >> I have thought for a while that, rather than writing more error-prone code<br> >> (at > 17k LOC, surely the API code is big enough as it is?), it would<br> >> be good to create a tool that helps us with the underlying problem -<br> >> incompatible changes made to marshaled types.<br> >><br> >> This would not be too hard - the Go language already has such a tool<br> >> (see <a href="http://golang.org/cmd/api/" target="_blank">http://golang.org/cmd/api/</a> ), although it is not currently generally<br> >> applicable.<br> >><br> >> This could make it possible to provide a much stronger assurance about<br> >> compatibility, including compatibility between types not defined inside<br> >> juju-core itself, while keeping the code natural and simple.<br> >><br> >> cheers,<br> >> rog.<br> >><br> ><br> > So that is interesting, and it might be fruitful to explore. I'll just note<br> > that it is actually a pretty small (and reasonably obvious) part of API<br> > compatibility. It *doesn't* tell you<br> ><br> > 1) If passing an empty string suddenly has a different meaning (see<br> > Client.ServiceSet where we used to treat "" as meaning reset-to-default vs<br> > Client.NewServiceSetForAPI where we wanted to actually treat "" as meaning<br> > set this value to the empty string.)<br> ><br> > 2) Returning a new value (Machine.Jobs(), we want to add new Jobs for new<br> > behaviors and don't want to confuse old agents until they have finished<br> > upgrading).<br> <br> </span>Yes, I agree. There are aspects of compatibility that<br> cannot be checked automatically, and we always need<br> to be careful when changing semantics. I don't<br> think there's any way around that.<br> <span class=""><br> > So while something like the above could tell you "has the shape of the API<br> > changed", and what I really like about it is that if it had a collapsed view<br> > (like a simple text representation), it is easy to keep that around in the<br> > source tree and use it for reference.<br> <br> </span>Yes, that's a nice thing about the Go api command, which does<br> use a simple text representation.<br> <span class=""><br> > However, you really do need tests for (1) and (2). (1) we actually broke<br> > accidentally because we changed the code inside of state/*, and then had to<br> > go back and retrofit the API to use a new code path to preserve<br> > compatibility.<br> <br> </span>Yup.<br> <span class=""><br> > And if you need tests, you really still need 2 types to represent the<br> > different versions, and if you already need those types, then I would<br> > recommend having a regular structure for them.<br> <br> </span>Here's where I don't agree. I don't think, for example, that it's worth<br> creating a new type just because its Jobs field can take on a value<br> that it could not previously have. We might need a new version of the API, sure<br> (although even that might be arguable in this case - agents just ignore<br> unknown job types), but defining a new type seems like overkill to me.<br> <br> Even when we do need to make a new type, I don't really see why<br> having a different type at each level will help us. One will probably<br> need to make the new type at every level too, so having the<br> differently defined types isn't a great benefit over having<br> a single changed type.<br> <br> There will of course be cases when we *do* want different types<br> at different levels, but that's easy to do on a case-by-case basis.<br> I suggest using appropriate types at the different levels<br> *as required by those levels*, rather than expending developer time and<br> generating extra runtime garbage by copying identical representations<br> of the same data around for every single type at every level.<br> <div class="HOEnZb"><div class="h5"><br> cheers,<br> rog.<br> <br> --<br> Juju-dev mailing list<br> <a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a><br> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a><br> </div></div></blockquote></div><br></div>