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

Fix runApplication handling of unrecognized command-line args #134

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

joseph-wakeling-frequenz
Copy link
Contributor

runApplication takes an args_out parameter which, if non-null, is supposed to be set to match unrecognized command-line args. However, the parameter is never passed to finalizeCommandLineOptions, which will therefore log an error and throw an exception if any command line arguments are unrecognized.

This patch fixes the oversight, ensuring that runApplication will correctly populate a non-null args_out parameter with unrecognized command-line arguments, and not throw an exception in this case.

`runApplication` takes an `args_out` parameter which, if non-null, is
supposed to be set to match unrecognized command-line args.  However,
the parameter is never passed to `finalizeCommandLineOptions`, which
will therefore log an error and throw an exception if any command line
arguments are unrecognized.

This patch fixes the oversight, ensuring that `runApplication` will
correctly populate a non-null `args_out` parameter with unrecognized
command-line arguments, and not throw an exception in this case.
@joseph-wakeling-frequenz
Copy link
Contributor Author

As a side remark: this design seems a bit intrusive in how it handles arguments. I get that the intent must be to allow vibe-core to ensure that it gets its hands on all its potential arguments before anyone else does (so someone can't getopt and clash with one of vibe's needed args), but as currently designed it looks like it's not readily possible to unittest, which is not great.

@joseph-wakeling-frequenz
Copy link
Contributor Author

What's not entirely clear to me is the intended usage of this args_out parameter, given that runApplication takes us in to the event loop and so marks the logical "end" of any main function (apart from post-eventloop-shutdown reporting). It would be good to have this documented.

I can make guesses about what that intended usage is, but it'd be good to have it clearly explained by someone behind the design of this code.

@s-ludwig
Copy link
Member

I agree about the lack of beauty of this functionality, it's a relic of the beginnings where the primary idea was to replicate the scripting language experience and to avoid any boilerplate code during initialization. The args_out followed later to work around its closed nature.

For runApplication, the only way to access the returned arguments would be in a separate task. I don't think this is actually used anywhere, but there are places where finalizeCommandLineArguments and runEventLoop are used explicitly. I think I just added the parameter for completeness sake when runApplication got introduced, without really thinking enough about it.

An immediate improvement would be to add a second overload of runApplication that takes a scope void delegate(string[]) nothrow @safe instead, so that it is at least clear when those arguments can be accessed. Long-term, I'd like to get rid of the command line argument handling altogether (moving it to an independent opt-in package, e.g. vibe-commandline).

@s-ludwig s-ludwig merged commit 2727780 into vibe-d:master Feb 22, 2019
@joseph-wakeling-frequenz joseph-wakeling-frequenz deleted the fix-run-app-args branch February 25, 2019 10:28
@joseph-wakeling-frequenz
Copy link
Contributor Author

@s-ludwig makes sense, thanks for giving the context and history.

If there's long-term scope for some significant re-thinking of how vibe-core works, I might have some interest in following up on that. I won't commit to anything now, but I may drop you a line some time in the future if this becomes more than just a line of thought.

@s-ludwig
Copy link
Member

I may be overlooking something now, but I think it really is just the command line handling, the main part of vibe-core is meant to remain in its current form for the time being. Do you have any thoughts about other areas?

@joseph-wakeling-frequenz
Copy link
Contributor Author

Nothing too explicit for now, as it's quite a speculative line of thought which needs to be properly researched before offering an opinion. But the core of it is about ensuring that we have a very nice, flexible, and simple-to-use async-io-based task system, with easy interactions with stuff like streams.

There are a few different implementations of that -- vibe.d, the task framework in sociomantic's ocean library, weka.io's mecca.reactor -- and it could be good to try to have a review of features, limitations, assumptions, etc. to see whether we can boil all those needs down to a common core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants