Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support building URLs with non-http schemes. #260

Merged
merged 4 commits into from
May 21, 2017
Merged

Support building URLs with non-http schemes. #260

merged 4 commits into from
May 21, 2017

Conversation

ChrisHines
Copy link
Contributor

  • Capture first scheme configured for a route for use when building URLs.
  • Add new Route.URLScheme method similar to URLHost and URLPath.
  • Update Route.URLHost and Route.URL to use the captured scheme if present.

Fixes #13. Supersedes #161.

@ChrisHines ChrisHines force-pushed the build-url-scheme branch 2 times, most recently from fe5fac8 to 299d70e Compare May 18, 2017 20:28
@elithrar elithrar requested review from kisielk and elithrar May 19, 2017 23:49
@kisielk
Copy link
Contributor

kisielk commented May 20, 2017

Looks good, but I'm not sure adding URLScheme is necessary. It's true that it seems to be symmetric with the rest of the API, but how useful is it really? I can't imagine it being used very often.

Also you could achieve the same thing by doing

schemeURL := &url.URL{Scheme: r.URL().Scheme}

in the case that you really need it..

@ChrisHines
Copy link
Contributor Author

Agreed. I was having doubts about URLScheme myself.

@ChrisHines
Copy link
Contributor Author

I'm trying to remove URLScheme, but it doesn't work as cleanly as I'd hope. Specifically the suggested work around of schemeURL := &url.URL{Scheme: r.URL().Scheme} doesn't work if the route doesn't have a host or path. Similarly, r.URLHost() returns an error if r doesn't have a configured host.

@ChrisHines
Copy link
Contributor Author

The testing is less elegant, but it works. PTAL.

ChrisHines added 3 commits May 20, 2017 22:20
@ChrisHines
Move misplaced tests and fix comments.
dac1bfc
@ChrisHines
Support building URLs with non-http schemes.
d62a8ce 
- Capture first scheme configured for a route for use when building
  URLs.
- Add new Route.URLScheme method similar to URLHost and URLPath.
- Update Route.URLHost and Route.URL to use the captured scheme if
  present.
@ChrisHines
Remove Route.URLScheme method.
a8bcb6a
@ChrisHines
Copy link
Contributor Author

Rebased on master to resolve conflicts in mux_test.go.

kisielk
kisielk approved these changes May 21, 2017
View reviewed changes
Copy link
Contributor

@kisielk kisielk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, apart from gofmt error for some reason.

@ChrisHines
Copy link
Contributor Author

Somehow mux_test.go had acquired a BOM.

@ChrisHines
Copy link
Contributor Author

Somehow mux_test.go had acquired a BOM.

The tool I used to resolve the conflict when rebasing was the source of the BOM. I've changed the settings on that so I won't have that problem again.

@elithrar elithrar merged commit bcd8bc7 into gorilla:master May 21, 2017
@elithrar
Copy link
Contributor

Merged. Thanks @ChrisHines!

@ChrisHines ChrisHines deleted the build-url-scheme branch May 21, 2017 12:38
This was referenced Jun 30, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kisielk kisielk kisielk approved these changes

@elithrar elithrar elithrar approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChrisHines @kisielk @elithrar