Adding period char to url regex in the makeHandler method.

[jackspirou]

This should fix #32.

[dmitshur]

See #32 (comment).

It appears periods are not allowed in usernames/organizations.

Also, this regex will match repo names "." and ".." both of which are not allowed, and would cause the url path to be misinterpreted anyway.

[jackspirou]

@shurcooL replied back #32 (comment)

Good catch on the usernames/org names. Also good catch on repos that simply consist of . or ... It would probably be ideal to filter those edge cases out.

I'll look at updating the pull request to better reflect what github allows in repo names.

[jackspirou]

@shurcooL I updated the pull request to account for the cases you pointed out. If you think there is a more clean way to filter out the . and .. cases please let me know (such as part of the regex). If not the if statement seems to do the job and is decently clean though I admit not totally ideal.

[dmitshur]

Are you sure makeHandler is used only for generating handlers that expect a name, followed by github username, then github repo?

It's not documented so it's hard to tell.

[jackspirou]

Line 30 fn(w, r, m[1], m[2]) seems to expect m[1] and m[2] to be available slice indices. Now if thats a good thing or not is a different question.

I assumed that line 17 validPath.FindStringSubmatch(r.URL.Path) returns nil if validPath can't produce a slice of m[0, 1, 2] so that is why line 30 can expect m[2] to be valid.

I say all this assuming the same things you mentioned above around github usernames and repos. 😅

[dmitshur]

To clarify my concern/question, you've added GitHub-specific rules to a func named makeHandler. If that func happens to be used to generate, for example, non-GitHub-related handlers, then it's not correct (because those non-GitHub-related handlers may not want to accept periods at all).

I'm not familiar with this codebase so I can only go based on the name of func and its documentation, which doesn't make it clear.

[jackspirou]

If you go to the webpage http://goreportcard.com/ it seems to only allow github repos.

My understanding after playing with this codebase is the makeHandler function creates all the handlers for other routes including /badge and /report, but those need a github username and repo as well. Im hoping the owners of this repo can shed light on if this change breaks anything, but so far in my local testing it looks good.

[dmitshur]

Great, thanks for confirming that. I was only 80% confident that was the case. :)

[dmitshur]

I'm really not a fan of regexes in general, so I'm all in favor of having logic moved outside as you've done. I think it's good.

LGTM.

[dmitshur]

then I accidentally uploaded the Mac OSX binary I compiled/built for testing.

Ah, I see. You just removed a file you inadvertently added as part of this PR. I missed that.

Personally I think a separate PR should be created to include the default binary build name goreportcard in the .gitignore file of this project so others don't repeat my mistake.

IMO that's not necessary. It's a pretty uncommon occurrence because you typically pick which files to commit, and it's easily caught during review. I personally prefer having as empty a .gitignore as possible, so any non-native files show up when I type git status rather than being invisible and increasing risk of introducing side-effects without me being aware. But that's just my preference.

Also, in order to save time, I typically do go build -o /tmp/o (or GOBIN=/tmp go install) rather than go build. That way I don't need to clean up my temporary binaries from current working directory.

[jackspirou]

Yes, I do tend to do go build and then rm binary a lot, thanks for the go build -o /tmp/o tip.

On a side note i'm interested to hear what operating specific files or non-native files you would not include within a .gitignore that could otherwise introduce unintended side effects to a project. Also, do you then not recommend the default .gitignore file contents github provides/suggests when starting a new project/repo?

Not trying to push back, just interested in your view point. 😃

[dmitshur]

I'm happy to share my view point.

On a side note i'm interested to hear what operating specific files or non-native files you would not include within a .gitignore that could otherwise introduce unintended side effects to a project.

One example I've seen that caused occasional trouble is the .gitignore that comes in in Godeps/_workspace directories, generated by godep. It ignores Godeps/_workspace/pkg and Godeps/_workspace/bin folders. Prior to 1.5, there was an issue where stale libraries could've been used when building. So there were instances of people having really strange behavior, and the fix was doing rm -rf Godeps/_workspace/pkg. They had a harder time figuring out that their Godeps/_workspace/pkg was non-empty because it's gitignored. I wouldn't neccessarily suggest removing that .gitignore, it's just an example.

Another example would be ignoring the binary itself (for convenience of being able to do go build, as you wanted), then doing go install and running the binary name, expecting the latest installed binary. If your PATH includes ., then the potentially old binary in cwd would be executed instead. Again, doing git status would not alert you about that.

Also, do you then not recommend the default .gitignore file contents github provides/suggests when starting a new project/repo?

It's up to you, for all my new Go packages, I start with a blank repository (without a .gitignore) file. It works great in my experience.

Take a look at my repositories. The only ones that have .gitignore files tend to be older C++/C#/Java projects. Those ones tend to benefit from .gitignore. With pure Go packages, I find .gitignore is not needed at all. My ideal Go package repository would have just .go files, everything else is a distraction. :P