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.

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

Allow listenHTTP to accept a convenience setting string #1816

Merged
merged 1 commit into from
Jul 5, 2017

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Jul 4, 2017

Follow-up to #1810

Now that HTTPServerSettings accept a string, listenHTTP can do so as well 🎉

Instead of adding five new overloads, I opted for templates.


shared static this()
{
listenHTTP(":11721", (scope req, scope res) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the hard-coded port from other tests - how about adding something like getFreePort to Vibe.d?

Copy link
Member

Choose a reason for hiding this comment

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

Or supporting ":0" and providing a way to retrieve the actual listen port.

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

I originally decided against adding this because of the already large number of overloads, but didn't think about converting them to templates to hide that. Should be okay like this.

@wilzbach
Copy link
Member Author

wilzbach commented Jul 5, 2017

s-ludwig added the auto-merge label 3 hours ago

@s-ludwig are you sure that you configured the webhook correctly here? I can't find any events in the log.
The config should look roughly like this:

image

@s-ludwig
Copy link
Member

s-ludwig commented Jul 5, 2017

Yay, finally worked!

@wilzbach wilzbach deleted the convenient-listen branch July 5, 2017 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants