Code review comment for lp:~vds/usso/support_sha1

Revision history for this message
Vincenzo Di Somma (vds) wrote :

> I'll try to review that in details later today but just a quick notes:
>
> [0]
>
> + fmt.Print("Enter signature method (PLAINTEXT or HMAC-SHA1): ")
> + fmt.Scanf("%s", &signature_method)
>
> How about we ditch that and perform the call for the two available signatures?
> The goal of this example is to run a test (that covers as much code as
> possible) against a real server in the quickest way possible.

It's just an example but I don't really see a reason for two http request.

> [1]
>
> 22 - fmt.Println("This application will query the staging Ubuntu
> SSO Server to fetch authorisation tokens.")
> 23 + fmt.Println("This application will query the staging Ubuntu
> SSO Server" +
> 24 + " to fetch authorisation tokens.")
>
> and
>
> 384 - server := newSingleServingServer("/api/v2/tokens",
> string(jsonServerResponseData), 200)
> 385 + server := newSingleServingServer("/api/v2/tokens",
> 386 + string(jsonServerResponseData), 200)
>
> and
>
> 159 +func (suite *USSOTestSuite) TestNormalizeURLLeavesNonstandardPort(
> 160 + c *gocheck.C) {
>
> etc.
>
> I think Jeroen's remark about the formatting is right. It's best if we try to
> use the same style as the other Go projects (lp:juju-core for instance) and
> use long lines instead of wrapping things like that.

I understand juju-core uses long lines, but omnibus, for example, doesn't. I only see cons in long lines and no pros.

« Back to merge proposal