-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: daily now uses the fact api can take multiple packages #3
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |
"encoding/json" | ||
"io/ioutil" | ||
"net/http" | ||
"strings" | ||
) | ||
|
||
type Daily struct { | ||
|
@@ -18,31 +19,36 @@ type DailyDownload struct { | |
Downloads int `json:"downloads"` | ||
} | ||
|
||
func GetDaily(period, pkg string) (Daily, error) { | ||
func GetDaily(period Period, pkgs []string) ([]Daily, error) { | ||
// weirdly the API returns an array of length 1 | ||
var daily []Daily | ||
|
||
path := URL + "downloads/daily/" + period + "/" + pkg | ||
|
||
path := URL + "downloads/daily/" + string(period) + "/" + strings.Join(pkgs, ",") | ||
resp, err := http.Get(path) | ||
|
||
if err != nil { | ||
return Daily{}, err | ||
return daily, err | ||
} | ||
// This is currently kind of meaningless until | ||
// https://github.com/r-hub/cranlogs.app/issues/41 is resolved but at least | ||
// its there | ||
if resp.StatusCode != 200 { | ||
return daily, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is.... bare minimum ok - ideally would consider more about what alternative responses could come in then handle them. One huge issue is essentially this will currently swallow the result that isn't an error. So for example, the boy json with the error message currently is being swallowed - so this needs to get refactored to likely generate a better, processed error. |
||
} | ||
|
||
defer resp.Body.Close() | ||
|
||
body, err := ioutil.ReadAll(resp.Body) | ||
|
||
if err != nil { | ||
return Daily{}, err | ||
return daily, err | ||
} | ||
|
||
err = json.Unmarshal(body, &daily) | ||
|
||
// if this errors given the current 200 status code returning an error response | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is now (partially) fixed due to the new deployment that does 404 (at least for some cases) |
||
// such as { "error": "Invalid query", "info": "https://github.com/metacran/cranlogs.app" } | ||
// its still not the best error code | ||
if err != nil { | ||
return Daily{}, err | ||
return daily, err | ||
} | ||
|
||
return daily[0], nil | ||
return daily, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because of that comma separated packages - keeps the same data structure if 1 package or many..... note: need to update to remove the comment now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know we could pass multiple packages