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

Cli parser fixups #116

Merged
merged 9 commits into from
Sep 29, 2020
Merged

Cli parser fixups #116

merged 9 commits into from
Sep 29, 2020

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Sep 28, 2020

Fixes #112

@bpkroth bpkroth requested review from amueller and a team September 28, 2020 19:55
@@ -26,6 +26,9 @@
"preLaunchTask": "", //"build",
"program": "${workspaceFolder}/out/dotnet/source/Mlos.Agent.Server/objd/AnyCPU/Mlos.Agent.Server.dll",
"args": [
//"--optimizer-uri",
//"http://localhost:50051",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixup for the default debugger experience.
If this options are enabled by default, the command will fail.

In the future we can consider to add a preLaunchTask that starts the optimizer service.


var cliOptsParseResult = CommandLine.Parser.Default.ParseArguments<CliOptions>(args)
var cliOptsParser = new Parser(with => with.HelpWriter = null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We add custom help output handling with ShowUsageHelp now.

/// See Also: https://github.com/microsoft/MLOS/issues/112.
/// </remarks>
[CommandLine.Value(0)]
public IEnumerable<string> ExtraArgs { get; set; }
Copy link
Contributor Author

@bpkroth bpkroth Sep 28, 2020

Choose a reason for hiding this comment

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

This option unfortunately isn't hidden and leaves a weird value pos. 0 text at the end of the help output, but it does allow us to detect the extra arguments.

@amueller
Copy link
Contributor

amueller commented Sep 28, 2020

This looks reasonable but you shouldn't trust my judgement of C#. My impulse (which you should feel free to ignore) would be to ask for a test to show that it errors on extra args.

@bpkroth
Copy link
Contributor Author

bpkroth commented Sep 29, 2020

This looks reasonable but you shouldn't trust my judgement of C#. My impulse (which you should feel free to ignore) would be to ask for a test to show that it errors on extra args.

Not a bad idea. Will have to think about how to implement that though.

- move cli parsing to a separate file
- change error message for unknown extra arguments
note that parsing of the right arguments already happens in other unit
tests

This invokation method appears to work on both windows and linux
@bpkroth
Copy link
Contributor Author

bpkroth commented Sep 29, 2020

This looks reasonable but you shouldn't trust my judgement of C#. My impulse (which you should feel free to ignore) would be to ask for a test to show that it errors on extra args.

Not a bad idea. Will have to think about how to implement that though.

Added this in 2ec178a

@bpkroth bpkroth merged commit 5936ceb into microsoft:main Sep 29, 2020
@bpkroth bpkroth deleted the cli-parser-fixups branch October 30, 2020 21:37
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.

Non-option arguments to MlosAgentServer are silently ignored
4 participants