vibe-d / vibe.d

Official vibe.d development
MIT License
1.15k stars 284 forks source link

CLI args for prominent HTTPServerSettings #2039

Open MartinNowak opened 6 years ago

MartinNowak commented 6 years ago

Basically any of my vibe-d app allows -b|--bind <addr> and -p|--port <port>. Time to add it to vibe-d's default arguments IMO.

https://github.com/MartinNowak/heroku-buildpack-d/pull/2

Would be great if there was a function to get a HTTPServerSettings instance that is configurable from the CLI.

auto settings = defaultHTTPServerSettings();

or maybe just do it in the constructor

auto settings = new HTTPServerSettings();

It should also integrate with getopt.

auto settings = new HTTPServerSettings();
auto helpInformation = getopt(
    args,
    "length",  &length,    // numeric
    "file",    &data,      // string
    settings.getoptArgs[],
    "verbose", &verbose,   // flag
);
s-ludwig commented 6 years ago

With --disthost and --distport (which currently don't do anything useful), as well as --uid and --gid, already present it's hard to argue against more (HTTP) server specific arguments. On the other hand it's kind of strange to have those by default in the first place for things like GUI applications that either don't start any server at all, or would simply be confused by the effects of those flags.

Adding the possibility to disable server specific flags could mitigate that, although it would be nicer to have that automated somehow. I don't think that there is a good heuristic for that, though, so opt-out or opt-in seem to be the only practical possibilities.

I'd separate getopt support out as an independent issue. This can be something that vibe.core.args supports generically.

As for the actual behavior, for the two basic approaches, change the default, or override the application chosen settings, the former is the only one where there is no risk to give the user a tool that can break the application's function, but on the other hand it requires the application to be conciously written in a way to support that command line argument, which kind of makes an argument for keeping it an application-specific setting in the first place. readOption("bind", &settings.bindAddresses[0]); readOption("port", &settings.port); is going to be shorter than having to detect whether a user supplied default was set and to set an application defined default other than [::]:80 instead.

Really not decided on which way to go overall, yet...