Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass golint for non-example packages #12

Merged
merged 5 commits into from
Dec 21, 2015

Conversation

buddhamagnet
Copy link
Contributor

All the non-example packages now pass golint, hope this helps!

The code under the examples folder needs more detailed attention, happy to assist. Love this, we are building out microservices at The Economist and it's fascinating to see what's going on. Here are a couple of our open source repos:

https://github.com/EconomistDigitalSolutions/ramlapi
https://github.com/EconomistDigitalSolutions/goberry

...more to come!

@@ -72,10 +74,12 @@ func (t *TestSubscriber) Start() <-chan pubsub.SubscriberMessage {
return msgs
}

// Err ertuens the GivenErrError value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple obvious spelling mistakes in here.

@@ -234,10 +234,10 @@ func (s *TestSQSAPI) GetQueueAttributesRequest(*sqs.GetQueueAttributesInput) (*r
func (s *TestSQSAPI) GetQueueAttributes(*sqs.GetQueueAttributesInput) (*sqs.GetQueueAttributesOutput, error) {
return nil, errNotImpl
}
func (s *TestSQSAPI) GetQueueUrlRequest(*sqs.GetQueueUrlInput) (*request.Request, *sqs.GetQueueUrlOutput) {
func (s *TestSQSAPI) GetQueueURLRequest(*sqs.GetQueueURLInput) (*request.Request, *sqs.GetQueueUrlOutput) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes golint pass, but actually breaks the code. AWS's library unfortunately generates names that do not match Go conventions 😞

https://godoc.org/github.com/aws/aws-sdk-go/service/sqs#GetQueueUrlInput

server.HTTPPort = 80
server.HTTPAccessLog = "LOG"
server.HealthCheckType = "PINGDOM"
portBound := server.PortBound()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you're missing the implementations of a few methods (PortBound, LogsPopulated, HealthCheckSet)

Not sure about adding these methods just to check for empty/nil values. It seems like it may add some unneeded misdirection, but I'll wait to see the full implementation before rejecting them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry didn't mean to add them, was in another branch!

@jprobinson
Copy link
Contributor

Just wanted to say thanks for the work you're doing! Very excited to see what you and the folks at The Economist come up with :)

@buddhamagnet
Copy link
Contributor Author

@jprobinson no hassles, we do use the Consul key-value store and parts (not all) of go-kit, and will be fascinated to see if gizmo fits into our picture.

@@ -8,7 +8,7 @@ import (
)

type (
// TestPublisher is a simple implementation of pubsub.Publisher meant to
// TestSubscriber is a simple implementation of pubsub.Publisher meant to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the name, but still has a bad doc. I'll fix it in master.

@jprobinson
Copy link
Contributor

thanks! 😸

jprobinson added a commit that referenced this pull request Dec 21, 2015
Pass golint for non-example packages
@jprobinson jprobinson merged commit e8e5da5 into nytimes:master Dec 21, 2015
@buddhamagnet
Copy link
Contributor Author

@jprobinson no worries, will have a deeper dig now!

@buddhamagnet buddhamagnet deleted the housekeeping/golint branch December 21, 2015 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants