Code review comment for lp:~prudhvikrishna/goamz/sns

This looks fantastic overall.

I have several points that are mainly related to conventions and idiomatic
coding for Go, but nothing really major or hard.

Please push it again and ping me once you have these ready, and let me
know if you have any questions on these.

[1]

> type Message struct {
(...)
> Message [8192]byte
(...)
> func (topic *Topic) Message(message [8192]byte, subject string) *Message {
> return &Message{topic.SNS, topic, message, subject}
> }

Both of these should use []byte rather than [8192]byte, and the field should
be named something like Payload instead of Message (Message.Message isn't
nice).

That said, why do we need the Message struct at all? It doesn't look
like it's used anywhere, so we can just drop it.

[2]

> type PublishOpt struct {
> (...)
> func (sns *SNS) Publish(options *PublishOpt) (resp *PublishResponse, err os.Error) {

This is one of the most interesting methods in the interface, and the function signature
feels a bit unfortunate to be using frequently. Let's organize it like this:

func (sns *SNS) Publish(message []byte, topic string)
func (sns *SNS) PublishSubject(message []byte, topic string, subject string)
func (sns *SNS) PublishStructure(message []byte, topic string, subject string, structure string)

The first two are actually backed by the last one.

[3]

> type Topic struct {
> *SNS
> TopicArn string
> }

s/TopicArn/ARN/

I've made the mistake of duplicating the type name in other places, but I want
to get it fixed at some point. The structure name is already Topic, so there's
no need to put Topic in the field name as well.

Also, let's please do s/Arn/ARN/ in the rest of the code in the Go side. This
is an acronym, so should be capitalized (like SNS itself).

[4]

> type Subscription struct {
> Endpoint string
> Owner string
> Protocol string
> SubscriptionArn string
> TopicArn string
> }

Similarly, s/SubscriptionArn/ARN/, and s/TopicArn/TopicARN/.

[5]

> type AttributeEntry struct {
> Key, Value string
> }
>
> type GetTopicAttributesResponse struct {
> Attributes []AttributeEntry `xml:"GetTopicAttributesResult>Attributes>entry"`
> ResponseMetadata
> }

Let's please try to avoid such long names:

s/Attributes/Attrs/
s/Response/Resp/

across the whole code, besides the necessary strings to make
Amazon happy, and ResponseMetadata which feels like a convenient
way to parse that recurring blob of data.

[6]

> type GetTopicAttributesResponse struct {
> Attributes []AttributeEntry `xml:"GetTopicAttributesResult>Attributes>entry"`

Then, the Attrs field in that type is actually a map, and users will be
much happier to handle it as such in the Go side. Define an internal struct
so that you can parse the XML out, and then internally translate it into
the following:

type GetTopicAttrsResp struct {
    Attrs map[string]string
    ResponseMetadata
}

[7]

> type SetTopicAttributesResponse struct {
> ResponseMetadata
> }
>
> type DeleteTopicResponse struct {
> ResponseMetadata
> }
> type RemovePermissionResponse struct {
> ResponseMetadata
> }
(...)

There are several such types which are exactly the same.

Let's please use the same convention used for EC2:

type SimpleResp struct {
    ResponseMetadata
}

Use this type wherever we have no other fields besides the Metadata itself.

[8]

> func (sns *SNS) ListTopics(NextToken *string) (resp *ListTopicsResponse, err os.Error) {
> resp = &ListTopicsResponse{}
> params := makeParams("ListTopics")
> if NextToken != nil {
> params["NextToken"] = *NextToken
> }

A string would be much nicer to use:

... ListTopics(NextToken string) ...
    ...
    if NextToken != "" {
        ...
    }

Please do the same for other functions using the same convention.

[9]

> func (sns *SNS) SetTopicAttributes(AttributeName, AttributeValue, TopicArn string) (resp *SetTopicAttributesResponse, err os.Error) {

Please update the signature:

func (sns *SNS) SetTopicAttrs(topic, name, value string) (resp *SetTopicAttrsResp, err os.Error) {

The ARN should be first, as it's the identifier of the thing we're modifying.

[10]

> if AttributeName == "" || TopicArn == "" {
> return nil, os.NewError("Invalid Attribute Name or TopicArn")
> }

if arn == "" || name == "" {
        return nil, os.NewError("topic ARN and attribute name must not be empty")
}

[11]

> func (sns *SNS) Subscribe(Endpoint, Protocol, TopicArn string) (resp *SubscribeResponse, err os.Error) {

Similarly:

func (sns *SNS) Subscribe(topic, endpoint, protocol string) (resp *SubscribeResp, err os.Error) {

[12]

> func (sns *SNS) ConfirmSubscription(options *ConfirmSubscriptionOpt) (resp *ConfirmSubscriptionResponse, err os.Error) {

Inline options here as well. There are just three of them.

[13]

> func (sns *SNS) AddPermission(permissions []Permission, Label, TopicArn string) (resp *AddPermissionResponse, err os.Error) {

We use Perm and Perms in EC2 already, and according to the above, we'd get:

func (sns *SNS) AddPerm(topic, label string, perms []Perm) (resp *AddPermResp, err os.Error) {

[14]

Please review other methods according to these suggestions above.

[15]

Documentation is very weak. Please see the other packages for some guidance
on what we need there.