Merge lp://staging/~julian-edwards/gwacl/xml-definitions into lp://staging/gwacl

Please consider the suggestions I have; apart from what I point out in [7] nothing is really *wrong* but I think we can improve things a bit:

[0]

Please use 'gofmt' to reformat the code. The structures with their "xml decorations" will be much more readable.

[1]

21 +func makeEndpoint(params endpointParams) *InputEndpoint {
[...]
40 +func makeLinuxProvisioningConfiguration(params provconfParams) *LinuxProvisioningConfiguration {
etc.

It's a detail but in Go, creator methods like this tend to use the 'New' prefix (instead of make); the typical declaration is: "NewT() *T". I know it does not matter much but it's good to follow the Go standards whenever it does not hurt.

[2]

149 +// It's impossible to have any kind of common method inherited into all the
150 +// various serializable objects because the receiver of the method is the
151 +// wrong class which confuses the xml marshaller. Hence, this mess.
152 +type AzureObject interface {
153 + Serialize() (string, error)
154 +}
155 +
156 +func toxml(obj AzureObject) (string, error) {
157 + out, err := xml.MarshalIndent(obj, "", " ")
158 + if err != nil {
159 + return "", err
160 + }
161 + return string(out), nil
162 +}
[...]
183 +func (c *InputEndpoint) Serialize() (string, error) {
184 + return toxml(c)
185 +}

You said you wanted to do this in order to have a Serialize() method on each object instead of having to call a global method. I agree with you in principle but here that it would be better. However, in Go, what you had to do in order to do this is add some boilerplate code that simply limits our ability to control how the xml is produced (by wrapping it in a method that does basically nothing but call xml.MarshalIndent()). I'll leave it up to you because you seemed to have very strong feelings about this but I'd advise getting rid of this completely and simply using xml.MarshalIndent() in the tests.

[3]

259 + c.Check(xml, Equals, fmt.Sprintf(`
260 +<LinuxProvisioningConfiguration>
261 + <ConfigurationSetType>LinuxProvisioningConfiguration</ConfigurationSetType>

If Gavin can write a Dedent() method, the definition of the expected XML in the tests will look much better.

[4]

205 +type Role struct {
206 + RoleSize string `xml:"RoleSize"`
207 + RoleName string `xml:"RoleName"`
208 + RoleType string `xml:"RoleType"` // Always "PersistentVMRole"
209 + ConfigurationSet []LinuxProvisioningConfiguration `xml:"ConfigurationSets>ConfigurationSet"`
210 +}

There is no support for default values of a field in a structure and this is a PITA. But I suggest creating a method with the following signature: "NewRole(roleSize string, roleName string, configurationSet []LinuxProvisioningConfiguration)" that will initialize the new struct's 'roleType' field with "PersistentVMRole".

Same for the other structures that have constant fields. I know that creating this code, for python programmers, seems like wasted time but AFAIK this is the only way to do this in Go and this way, you encapsulate that knowledge in the code and can test for it, instead of expecting the caller to do the right thing based on a comment in the code.

[5]

134 func init() {
...

Thanks for looking at the branch Raphers.

On 11/03/13 21:37, Raphaël Badin wrote:
> Please consider the suggestions I have; apart from what I point out in [7] nothing is really *wrong* but I think we can improve things a bit:
>
> [0]
>
> Please use 'gofmt' to reformat the code. The structures with their "xml decorations" will be much more readable.

I've done this. It's screwed up other formatting but I am beyond caring.

>
> [1]
>
> 21 +func makeEndpoint(params endpointParams) *InputEndpoint {
> [...]
> 40 +func makeLinuxProvisioningConfiguration(params provconfParams) *LinuxProvisioningConfiguration {
> etc.
>
> It's a detail but in Go, creator methods like this tend to use the 'New' prefix (instead of make); the typical declaration is: "NewT() *T". I know it does not matter much but it's good to follow the Go standards whenever it does not hurt.

Hmmm. I wanted to make a test factory-only method. Our convention for
those is to have makeXXX. In particular, I do not want to export these
methods for outside use at all, since they generate random values.

> [2]
>
> 149 +// It's impossible to have any kind of common method inherited into all the
> 150 +// various serializable objects because the receiver of the method is the
> 151 +// wrong class which confuses the xml marshaller. Hence, this mess.
> 152 +type AzureObject interface {
> 153 + Serialize() (string, error)
> 154 +}
> 155 +
> 156 +func toxml(obj AzureObject) (string, error) {
> 157 + out, err := xml.MarshalIndent(obj, "", " ")
> 158 + if err != nil {
> 159 + return "", err
> 160 + }
> 161 + return string(out), nil
> 162 +}
> [...]
> 183 +func (c *InputEndpoint) Serialize() (string, error) {
> 184 + return toxml(c)
> 185 +}
>
> You said you wanted to do this in order to have a Serialize() method on each object instead of having to call a global method. I agree with you in principle but here that it would be better. However, in Go, what you had to do in order to do this is add some boilerplate code that simply limits our ability to control how the xml is produced (by wrapping it in a method that does basically nothing but call xml.MarshalIndent()). I'll leave it up to you because you seemed to have very strong feelings about this but I'd advise getting rid of this completely and simply using xml.MarshalIndent() in the tests.

I genuinely don't understand what your suggestion buys us. Feel free to
convince me more, but I want to see concrete advantages. As it stands,
the way it's done encapsulates some formatting (IME this is *extremely*
important for debugging at a later date) and stringifies the output from
Marshal[Indent] because by default it gives you a byte array (and I
really don't understand how that is useful and why they did it like that).

> does basically nothing but call xml.MarshalIndent()

It also stringifies it.

> limits our ability to control how the xml is produced

It doesn't limit anything. You're free to reformat the xml if you're
crazy enough :)

In addition, having a single point through which stuff is converted to
xml means we can change it in one place and it affects all the objects,
keeping them consistent.

> [3...

Thanks for all the changes! (I forgot to approve that branch yesterday ;))

> I genuinely don't understand what your suggestion buys us. Feel free to
> convince me more, but I want to see concrete advantages. As it stands,
> the way it's done encapsulates some formatting (IME this is *extremely*
> important for debugging at a later date)

My point was that, by encapsulating the creation of the xml in one method like that, we force all the callsites to use the same formatting. If we were not doing that we could: use the indented version in the tests for maximum readability, use the non-indented version in production where 'debug' (a config setting somewhere) is off, use the indented version in production where 'debug' is one. What I'm saying is that the code right now slightly limits our ability to adapt the formatting to different situations.

And you'll admit that the conversion to a string is a one-liner and does not deserve a method in itself.

But ok, I guess we can revisit this code if we need the flexibility I'm talking about later on.

> I've done this. It's screwed up other formatting but I am beyond caring.

You're coming to the dark side… good :)

> It's staggering insofar as it's the first language I've used that
> doesn't preseed the random number generator.

I wasn't really a surprise to me because it reminded me how C works:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

void main(void) {
        /* srand ( time(NULL) ); uncomment this to seed the PRNG */
        printf("Random %d", rand());
}

On 12 March 2013 08:23, Raphaël Badin <email address hidden> wrote:
>> I genuinely don't understand what your suggestion buys us. Feel free to
>> convince me more, but I want to see concrete advantages. As it stands,
>> the way it's done encapsulates some formatting (IME this is *extremely*
>> important for debugging at a later date)
>
> My point was that, by encapsulating the creation of the xml in one
> method like that, we force all the callsites to use the same
> formatting. If we were not doing that we could: use the indented
> version in the tests for maximum readability, use the non-indented
> version in production where 'debug' (a config setting somewhere) is
> off, use the indented version in production where 'debug' is
> one. What I'm saying is that the code right now slightly limits our
> ability to adapt the formatting to different situations.

I think it's better to stick to one representation, and the pretty
version would be my choice. There's no need for the condensed version
because we can always use compression over the wire. The complexity of
adding a config option, while small, is not compensated by the limited
usefulness of being able to choose between pretty and ugly printing.
The former will also come in handy when debugging a live system.

On 12 March 2013 08:24, Raphaël Badin <email address hidden> wrote:
>> I've done this. It's screwed up other formatting but I am beyond caring.
>
> You're coming to the dark side… good :)

He's still using exotic gofmt options ;) Spaces, while obviously the
better choice (I used to be on the side of tabs, but I have realised
the error of my ways), are not the standard in Go. As with much in Go,
it is sometimes necessary to bite the pillow and think of
$YourHomeCountry to get ahead.

Perhaps this can been seen as a portend for the future
picking-up-the-soap-in-a-prison-shower scenarios that Go has planned
for its acolytes?

On 12/03/13 18:32, Raphaël Badin wrote:
>> It's staggering insofar as it's the first language I've used that
>> doesn't preseed the random number generator.
>
> I wasn't really a surprise to me because it reminded me how C works:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <time.h>
>
> void main(void) {
> /* srand ( time(NULL) ); uncomment this to seed the PRNG */
> printf("Random %d", rand());
> }
>

I don't remember having to do that in C before. Perhaps it's a Gnu libc
thing? Or perhaps I used a different rand. I dunno :)

On 12/03/13 19:49, Gavin Panella wrote:
> On 12 March 2013 08:24, Raphaël Badin <email address hidden> wrote:
>>> I've done this. It's screwed up other formatting but I am beyond caring.
>>
>> You're coming to the dark side… good :)
>
> He's still using exotic gofmt options ;) Spaces, while obviously the

Exotic to the likes of the Go devs it seems... sheesh.

> better choice (I used to be on the side of tabs, but I have realised
> the error of my ways), are not the standard in Go. As with much in Go,
> it is sometimes necessary to bite the pillow and think of
> $YourHomeCountry to get ahead.

I reckon we can do it how the $expletive we like for now and then
reformat when it gets handed over ;)

> Perhaps this can been seen as a portend for the future
> picking-up-the-soap-in-a-prison-shower scenarios that Go has planned
> for its acolytes?

/me thinks about writing a Py2Go.