Add Campaigns for Apps endpoints #1
Conversation
The package name was no longer considered mature. Also to align with the clients for other languages, the name of the package was changed. Next to that the location of the repo has also changed. While at it, the copyright year has been adjusted to 2018 were applicable.
After new endpoints have been made available to retrieve App Campaigns and their results, the client is adjusted to support these new endpoints.
vox-eve
requested review from
gPinato,
kioli,
Rifinio,
spirosikmd and
TomPaeta
|
|
||
| err := json.Unmarshal(data, &response) | ||
| if err != nil { | ||
| return response, err |
There was a problem hiding this comment.
Is by convention that we always return the response even though an error occurred?
(I ask since when an error is not present we return it with nil, but I guess it does not matter much since we check if the error is present and otherwise we process the answer)
There was a problem hiding this comment.
It's sort of a convention yes. That way on the other side you can do a
if err != nil {
...
}or look at the http module on how this is sort of a convention.
| ) | ||
|
|
||
| var ( | ||
| appFeedbackURI = appsURI + "/%s/feedback" | ||
| appFeedbackURI = appsURI + "/%s/feedback" |
There was a problem hiding this comment.
why there are so much space between appFeedbackURI and the '='
There was a problem hiding this comment.
That's the formatting Go uses, see documentation.
| // appCampaignResults feeds the results channel with items. While HasMore is true it makes new API requests | ||
| // and sends them through the channel. Once it is false it closes the channel. | ||
| func appCampaignResults(acrc chan AppCampaignResultStruct, resp *AppCampaignResultsResponse, acr *AppCampaignResults, campaignID string) { | ||
| for { |
There was a problem hiding this comment.
it feels like it is an infinite for. Is it correct practice ? Could you use a while or something safer to break this look ? (And I have no idea if it's possible too)
There was a problem hiding this comment.
Go only knows about a for loop, it contains all loop functionality in a single keyword. See documentation
|
|
||
| acrc := make(chan AppCampaignResultStruct) | ||
|
|
||
| go appCampaignResults(acrc, resp, ac, campaignID) |
There was a problem hiding this comment.
I agree, but it's sort of a Go convention to not use long variable names, see common codereview comments.
| func (acr *AppCampaignResults) Get(campaignID string, params map[string]string) (*AppCampaignResultsResponse, error) { | ||
| uri := fmt.Sprintf(appCampaignResultsURI, campaignID) | ||
|
|
||
| request := &request{ |
There was a problem hiding this comment.
pure noob question, should we add a space before the {
There was a problem hiding this comment.
My editor runs gofmt on the code when saving, so this is how it should be, read about gofmt to see what it does.
Reviewers: @spirosikmd @tzengerink @Rifinio @kioli @SuzettePaeta @gPinato
This PR does a couple of things, that I neatly arranged in their own commits.
Rename repo and package:
It renames the repository to 'api-go' to match our other clients and it renames the package to 'usabilla'.
Campaigns for Apps
It adds support for the new Campaigns for Apps endpoints. While at it I notice there is a lot of duplicate code and it would be nice to refactor that into DRYer and more maintainable code, which also allows for better testing of new endpoints. For now, however, the main goal is to support the new endpoints. I would like to consider refactoring the client as technical debt.
CI
It moves the continuous integration to CircleCI, like our other projects.