Add aggregator

[faktas2]

This is one way I thought we could use to decrease the amount of requests we make to PT. It uses the aggregator to make the GET requests in bulk.

Below is how it works,

	ptClient := pivotal.NewClient("TOKEN")
	ptClient.Aggregator.QueueStoryByID(557367, 181915841)
	ptClient.Aggregator.QueueStoryByID(557367, 182382842)
	ptClient.Aggregator.QueueStoryCommentsByID(557367, 181915841)
	ptClient.Aggregator.QueueStoryCommentsByID(557367, 179601257)
	ptClient.Aggregator.QueueReviewsByID(557367, 179601257)
	ptClient.Aggregator.FinishRequest()

	storyMap, _ := ptClient.Aggregator.GetStoriesFromRequest()
	commentsMap, _ := ptClient.Aggregator.GetStoryCommentsFromRequest()
	reviewsMap, _ := ptClient.Aggregator.GetStoryReviewsFromRequest()

[oschwald]

This pattern seems different than anything else this package does. In general, I would recommend making APIs immutable whenever possible as it reduces the chance for errors. In particular, this design seems prone to race conditions if used from multiple goroutines and requires a somewhat awkward InitRequest and FinishRequest. I think this could be cleaner if you built up the request using the builder pattern or just made an GetAggregate (or whatever) method that had variadic arguments of a particular type (e.g., functional options).

The value returned by this method could then have the getter methods you have below.

I think in this case, that would mean something

[faktas2]

Yes, I didn't think about that. I have changed it to a builder model. What do you think?

Now below is how it would be used:

ptClient := pivotal.NewClient("TOKEN")

storiesToGet := []int{
    181915841,
    182087301,
    182337490,
    182276377,
    181814584,
}
projectID := 557367
aggregation, _ := ptClient.Aggregator.BuildStoriesCommentsAndReviewsFor(projectID, storiesToGet)

story, _ := aggregation.GetStory(181915841)
comments, _ := aggregation.GetComments(181915841)
reviews, _ := aggregation.GetReviews(181915841)

[oschwald]

I think this is an improvement, but I still think it needs some work if it is something we want upstream to consider incorporating in the future. By builder, I meant the builder design pattern, e.g.:

var r ptClient.Aggregator.Request
agg, httpResponse, err := ptClient.Aggregator.Getter().Story(557367, 181915841).Story(557367, 182337490).Project(557367).Me().Send()

This would allow the easy extension to all of the different data supported by the API. (That said, I think it is fine to only support stories for now.)

I think the returned results should use the same model classes that the rest of the client API uses, e.g., *Story. It should probably retain the order of the responses and have some convenience methods to do things like get the story by the project ID and story ID.

[faktas2]

I have changed it. I've also added getting the stories by project ID and story ID.
Here is how it works now:

	ptClient := pivotal.NewClient("TOKEN")

	storiesToGet := []int{
		181915841,
		182087301,
		182337490,
		182276377,
	}
	projectID := 557367
	aggregation := ptClient.Aggregator.GetBuilder().Stories(projectID, storiesToGet).Story(projectID, 181814584)
	aggregation.CommentsOfStories(projectID, storiesToGet).ReviewsOfStories(projectID, storiesToGet)
	aggregation, err := aggregation.Send()

	story, _ := aggregation.GetStory(projectID, 181915841)
	comments, _ := aggregation.GetComments(projectID, 181915841)
	reviews, _ := aggregation.GetReviews(projectID, 181915841)

[oschwald]

I might also suggest submitting this upstream, https://github.com/salsita/go-pivotaltracker, to see if they have feedback. I am not too hopeful as I am not sure it is being actively maintained right now, but ideally this would be incorporated there or in a maintained fork in the future.

[oschwald]

A couple of more small comments and one comment from earlir.

[oschwald]

I think my previous comments were lost on a rebase or something, but this should return *http.Response as the second return value to match the rest of the API in this module.

[oschwald]

Ah, ok. Maybe just ignore this comment then. We don't need it ourselves. If upstream ever shows a willingness to merge this, we can deal with the concern then.

[faktas2]

I send bulks of 15 requests in Send. So, there are multiple http.Responses.

In my next commit, I've changed Send to only send the next bulk in the aggregation and return the corresponding http.Response. And then I've added another SendAll method, which sends all the bulks in one go. SendAll returns the http.Responses in a slice. Do you think that is fine?

[oschwald]

GetStoryByID might be a bit less wordy and seems clear enough. If you update this, also update the private function for building the URL.

[oschwald]

We should return an error if this fails. Same with the other json.Marshal calls.

I wonder if it would make sense to wrap this in a helper function, e.g.:

func mapTo(from interface{}, to interface{}) error {
    b, err := json.Marshal(from)
    if err != nil {
         return err
    }
    return json.Unmarshal(b, &to)
}

This should reduce the repeated code and in the future might make it easier to replace this with something more efficient, e.g., https://github.com/mitchellh/mapstructure.