Fix so that BaseURL can be used without the trailing slash

[178inaba]

If there is no trailing slash in BaseURL, I think that it is ideal to have the same result as when there is a trailing slash.

[dmitshur]

I don't think it's valid to change c.UploadURL for c.BaseURL.

[dmitshur]

I would prefer a solution that checks and reports a helpful error message if the BaseURL doesn't have a trailing slash. It's easy, and can be done in NewRequest like this:

if !strings.HasSuffix(c.BaseURL.Path, "/") {
    return nil, fmt.Errorf("BaseURL must have a trailing slash, but %q does not", c.BaseURL.String())
}
u := c.BaseURL.ResolveReference(rel)

Currently, there's only one canonical way to specify a BaseURL–that is with the trailing slash. I think that's a valuable property and worth preserving.

If you remove that requirement, there will be more than one way to specify a BaseURL. I don't think that's an improvement.

I say this because I don't think it's any harder to specify a URL with a trailing slash than without, so trying to do it automatically doesn't gain much convenience for users.

[178inaba]

Thank you so much for review!
I will follow your plan.
I will submit another pull request quickly.

[dmitshur]

Sounds good. Thank you @178inaba!

[gravis]

This is a breaking change, and a nasty one.
We discovered some issues with github syncing, and found this change in our deps.
While it sounds a good idea to return an error, this behaviour is not backward compatible with existing implementations.
Worse: The issue only occurs at runtime!

[dmitshur]

Hi @gravis, thanks for reporting this, it's very helpful for us.

Just to note, this PR was not merged, but #692 was merged instead. Did you mean to post this there?

While it sounds a good idea to return an error, this behaviour is not backward compatible with existing implementations.

Note that Client.BaseURL documentation says:

// Base URL for API requests. Defaults to the public GitHub API, but can be
// set to a domain endpoint to use with GitHub Enterprise. BaseURL should
// always be specified with a trailing slash.
BaseURL *url.URL

The relevant part is that "BaseURL should always be specified with a trailing slash." This was the API for a while, and PR #692 did not change that. If the BaseURL is specified without a trailing slash, there may be other issues as a result of that, because the code assumes it has a trailing slash. Can you share more details about your issue, what was the value BaseURL set to before and after?

[gravis]

I think the comment is wrong then, BaseURL must be specified with a trailing slash.
We initiate the client with https://api.github.com (without the trailing slash), and started to see this issue since a few days. We added some code to automatically add the slash if not present, but for sure other projects will have the same issue :(

[dmitshur]

You're right, the comment should say "must". Saying "should" is not accurate.

We initiate the client with https://api.github.com (without the trailing slash), and started to see this issue since a few days.

Actually, the root path is the only situation where it's actually harmless to leave out the trailing slash, the go-github code will operate successfully. That explains how it was working successfully for you before that change started to report an error.

I have another question for you. Did you know that https://api.github.com/ is already the default base URL of a client created with github.NewClient, as you can see here:

go-github/github/github.go

Line 31 in fe4b603

defaultBaseURL = "https://api.github.com/"

Why did you try to set BaseURL yourself to the same https://api.github.com/ value?

I ask because if it's not clear that BaseURL only needs to be set for GitHub Enterprise setups or other custom needs, we might want to consider improving documentation so it is more clear.

[gravis]

Because we support GitHub Enterprise too ;)

[KE7]

Go creates servers without the trailing slash (https://golang.org/src/net/http/httptest/server.go, Line 27).

@gravis was right in that this is a breaking change.

[gmlewis]

Yes, but I believe this was addressed in #692.

[KE7]

Yeah it gives you the error but I guess it's unfortunate that there is an inconsistency and you have to add random slashes