-
Notifications
You must be signed in to change notification settings - Fork 3
add a script for finding popular, but unused projects #29
base: master
Are you sure you want to change the base?
Conversation
Here's an example run:
|
Is this useful so far? I can easily add some sorting. |
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.
Looks nice. I'd like to request a few minor adjustments before we merge, though.
Besides the comments in the review, please squash the PR to one commit too.
Thanks.
main.go
Outdated
if err != nil { | ||
return -1, fmt.Errorf("problem loading %s: %v", req.URL, err) | ||
} | ||
if resp.Body != 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.
The net/http
documentation says "The http Client and Transport guarantee that Body is always non-nil, even on responses without a body or responses with a zero-length body."
I don't think we need this if statement.
main.go
Outdated
|
||
doc, err := html.Parse(resp.Body) | ||
if err != nil { | ||
panic(err) |
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.
Let's just return the error here. No need to panic.
main.go
Outdated
func scrapeGodocImports(importPath string) (int, error) { | ||
req, err := http.NewRequest("GET", "https://godoc.org/"+importPath, nil) | ||
if err != nil { | ||
panic(err) |
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.
Return -1, err
please, instead of panicking.
main.go
Outdated
}, | ||
}) | ||
if err != nil { | ||
panic(err) |
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.
Let's log err
to standard error and bail out more gracefully than this. Avoid panic
please.
main.go
Outdated
} | ||
} | ||
|
||
func createGithubClient(ctx context.Context) *github.Client { |
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 think this function should return an error. Currently, it prints a message to standard error if the github token isn't available, but then it continues creating the client regardless.
main.go
Outdated
var ( | ||
ctx = context.Background() | ||
|
||
ghClient = createGithubClient(ctx) |
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.
Move this inside of main
, please, and let's handle the error it will return more gracefully, by bailing out, instead of continuing.
main.go
Outdated
) | ||
|
||
var ( | ||
ctx = context.Background() |
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 think we should avoid this. For now, let's pass context.Background()
to all things that need it, explicitly.
If the context we use changes in the future, it'll be easier to spot at a glance and adjust.
Oh, something else too. I only just now noticed that this vendors dependencies, but it is using modules as well. Isn't that strange? I thought modules and vendoring were incompatible. |
614b967
to
08a6a8c
Compare
@acln0 Thanks for the review. I dropped |
There are a lot of -1's returned still, so I should probably look into
that. I hope we can just make that refresh call and retry.
|
Thanks. This looks pretty good now. I guess the next question is if we want to have a Ping @theckman for review and further thoughts. |
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.
If the option is between using modules and vendoring, I'd prefer we do what we need to have a functioning vendor directory. I'd like to make sure the binary is buildable no matter the state of any of its dependencies.
Can you please update this PR to add a vendor directory that ensures the script will build?
main.go
Outdated
@@ -0,0 +1,115 @@ | |||
package main |
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.
Let's make this cmd/finder/main.go
or something similar.
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.
Maybe add a // import "github.com/gofrs/help-requests/cmd/finder"
comment here too?
Perhaps a package-level comment would be nice too, like here https://github.com/golang/tools/blob/master/cmd/stringer/stringer.go
08a6a8c
to
7220b86
Compare
7220b86
to
35923ca
Compare
Added Edit: I updated the example: #29 (comment) |
popular
: the more godoc importers the more popularunused
: long time since last commit? few issue updates/commentsIssue: #14