`ArgParser` rewrite #3946

YoshiRulz · 2024-06-11T23:50:05Z

~~I'd still like to improve the documentation further, but this is functional already.~~ If you have scripts for e.g. encoding please test them.

Don't merge without checking the commit hash in README.md.

Also, it's kebab case, not snake case 🤦

src/BizHawk.Client.Common/ArgParser.cs

Morilli · 2024-06-12T12:33:26Z

If we do this, is there a way to get rid of the ParsedCLIFlags struct as well? Feels like unnecessary duplication.

YoshiRulz · 2024-06-12T15:00:05Z

If we do this, is there a way to get rid of the ParsedCLIFlags struct as well? Feels like unnecessary duplication.

~~Yes, by exposing the Options, returning the ParseResult, and moving the GetValueForOption calls throughout MainForm. I will try to do that for this PR.~~ edit: Actually, MainForm only uses a few flags on init, most of them are read in (methods called from) the main loop, so they'd need to be memoised anyway. And errors from our manual parsing would possibly happen later. So I don't think it's a good idea.

YoshiRulz · 2024-07-04T13:29:07Z

Should --help group the flags semantically? (Re-ordering the list is trivial, unfortunately adding headers isn't.) I chose to list them alphabetically since that's what I'm used to from Unix tools.

Morilli

This is probably an improvement for end-users (well, assuming you get --help to actually display stuff on windows) and about neutral for understanding code flow (it's a complete mess still).

Morilli · 2024-07-08T00:00:56Z

src/BizHawk.Client.Common/ArgParser.cs

 	public static class ArgParser
 	{
-		/// <exception cref="ArgParserException"><c>--socket_ip</c> passed without specifying <c>--socket_port</c> or vice-versa</exception>
-		public static void ParseArguments(out ParsedCLIFlags parsed, string[] args)
+		private sealed class BespokeOption<T> : Option<T>


Can you explain why this class is necessary?

BizHawk/src/BizHawk.Client.Common/ArgParser.cs

Lines 127 to 131 in dfe4a72

GeneratedOptions = root.Options.Where(static o =>

{

var t = o.GetType();

return !t.IsGenericType || t.GetGenericTypeDefinition() != typeof(BespokeOption<>); // no there is no simpler way to do this

}).ToArray();

(essentially root.Options.Where(o => o is not BespokeOption<>); this is necessary for --help as you already noted in another comment)
The other way, possibly the intended way, is to positively identify the HelpOption, but that type isn't exposed so you'd be comparing qualified names.

In my testing checking for Option<> seems to filter out the HelpOption, did you try that?

Sorry, I missed this comment. What exactly did you try?

-var t = o.GetType(); -return !t.IsGenericType || t.GetGenericTypeDefinition() != typeof(BespokeOption<>); +return o is Option<>;

...doesn't compile.

return typeof(Option<>).IsInstanceOfType(o);

...compiles but matches nothing.

Morilli · 2024-07-08T00:01:38Z

README.md

@@ -199,7 +199,8 @@ tl;dr:

 #### Passing command-line arguments

-EmuHawk takes some command-line options which aren't well-documented; you might be able to figure them out from [the source](https://github.com/TASEmulators/BizHawk/blob/78daf4913d4c8e47d24fc14d84ca33ddef913ed4/src/BizHawk.Client.Common/ArgParser.cs).
+EmuHawk takes some command-line options which, starting from 2.9.2, can be listed with `--help`.


This doesn't actually work on windows release builds due to the WinExe target, so this will need some consideration.

Morilli · 2024-07-08T00:04:01Z

src/BizHawk.Client.Common/ArgParser.cs

+//				.UseVersionOption() // "cannot be combined with other arguments" which is fair enough but `--config` is crucial on NixOS
+				.UseHelp()
+//				.UseEnvironmentVariableDirective() // useless
+				.UseParseDirective()
+				.UseSuggestDirective()
+//				.RegisterWithDotnetSuggest() // intended for dotnet tools
+//				.UseTypoCorrections() // we're only using the parser, and I guess this only works with the full buy-in
+//				.UseParseErrorReporting() // we're only using the parser, and I guess this only works with the full buy-in
+//				.UseExceptionHandler() // we're only using the parser, so nothing should be throwing
+//				.CancelOnProcessTermination() // we're only using the parser, so there's not really anything to cancel
+				.Build();


Not sure if it's necessary to comment all options that you're not setting, but whatever. What do the Use*Directive calls actually accomplish though? From the very useful documentation I found they supposedly enable certain commandline syntax, but also it seems to require EnableDirectives and even when setting that, I couldn't get the syntax to work, so... ?

Also, what's with the version option?

To explain, this list is the expansion of UseDefaults. I commented the ones I didn't use so that, if and when the list changes, there'll be a record of what didn't work in the past.
I'll be honest, I didn't play with the directives. I left them in just to follow the .NET convention and in the hope that someone would find a use for them.
We already have a --version which works well enough. When I (deleted that and) enabled UseVersionOption, it failed because it saw --config=.../config.json --version due to the launch script I use.

Morilli · 2024-07-08T00:34:40Z

src/BizHawk.Client.Common/ArgParser.cs

+			var triggeredGeneratedOption = GeneratedOptions.FirstOrDefault(o => result.FindResultFor(o) is not null);
+			if (triggeredGeneratedOption is not null)
+			{
+				// means e.g. `./EmuHawkMono.sh --help` was passed, run whatever behaviour it normally has...
+				var exitCode = result.Invoke();
+				// ...and maybe exit
+				if (exitCode is not 0
+					|| triggeredGeneratedOption.Name is "help") // `Name` may be localised meaning this won't work? I can't grok the source for `HelpOption`
 				{
-					cmdRom = arg;
+					return exitCode;


This looks kinda ugly... is there no way to standardize this into a single Invoke() call? I'm assuming not because the way bizhawk's argument parsing system works is so fundamentally different from what the CommandLine library expects.

Maybe a first step could be to separate long-term options that must stay for the entirety of the program's lifetime and could be stored in a struct like what already exists, and options that could be passed to the mainform directly. I don't really know, this just feel weird.

I mainly dislike the fact that trying to figure out how certain arguments are used and passed through takes multiple redirection steps: Search for --dump-name -> find OptionAVDumpName -> it gets passed to new ParsedCLIFlags as cmdDumpName argument -> the constructor sets the field this.cmdDumpName -> check where that field is used.

I mainly dislike the fact that trying to figure out how certain arguments are used and passed through takes multiple redirection steps: Search for --dump-name -> find OptionAVDumpName -> it gets passed to new ParsedCLIFlags as cmdDumpName argument -> the constructor sets the field this.cmdDumpName -> check where that field is used.

I could rename the options to match the ParsedCLIFlags fields, or better yet, rename the fields to these more descriptive names. I was hesitant to do that initially because I wanted to avoid churn in MainForm.

This looks kinda ugly... is there no way to standardize this into a single Invoke() call? I'm assuming not because the way bizhawk's argument parsing system works is so fundamentally different from what the CommandLine library expects.

That's it exactly. The library assumes it will be your Program implementation, and devolve its execution privilege to your peasant code. And also it heavily uses async. See the example here: https://learn.microsoft.com/en-us/dotnet/standard/commandline/handle-termination

Morilli reviewed Jun 12, 2024

View reviewed changes

src/BizHawk.Client.Common/ArgParser.cs Outdated Show resolved Hide resolved

YoshiRulz added 6 commits July 8, 2024 04:05

Document bare minimum of CLI flag parsing behaviour

d515b1b

Replace implementation of ArgParser using System.CommandLine

6c761ff

Add --help command-line flag

dfe4a72

Improve documentation of CLI flags (output of --help)

f1c624f

Standardise command-line flags to snake-case

989e012

Update readme section re: CLI

031596a

YoshiRulz force-pushed the cli branch from 6074f62 to 031596a Compare July 7, 2024 19:03

YoshiRulz marked this pull request as ready for review July 7, 2024 19:03

Morilli reviewed Jul 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ArgParser` rewrite #3946

`ArgParser` rewrite #3946

YoshiRulz commented Jun 11, 2024 •

edited

Loading

Morilli commented Jun 12, 2024

YoshiRulz commented Jun 12, 2024 •

edited

Loading

YoshiRulz commented Jul 4, 2024

Morilli left a comment

Morilli Jul 8, 2024

YoshiRulz Jul 8, 2024

Morilli Jul 8, 2024

YoshiRulz Jul 26, 2024

Morilli Jul 8, 2024

Morilli Jul 8, 2024

YoshiRulz Jul 8, 2024

Morilli Jul 8, 2024

YoshiRulz Jul 8, 2024

YoshiRulz Jul 8, 2024

	GeneratedOptions = root.Options.Where(static o =>
	{
	var t = o.GetType();
	return !t.IsGenericType \|\| t.GetGenericTypeDefinition() != typeof(BespokeOption<>); // no there is no simpler way to do this
	}).ToArray();

ArgParser rewrite #3946

Are you sure you want to change the base?

ArgParser rewrite #3946

Conversation

YoshiRulz commented Jun 11, 2024 • edited Loading

Morilli commented Jun 12, 2024

YoshiRulz commented Jun 12, 2024 • edited Loading

YoshiRulz commented Jul 4, 2024

Morilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`ArgParser` rewrite #3946

`ArgParser` rewrite #3946

YoshiRulz commented Jun 11, 2024 •

edited

Loading

YoshiRulz commented Jun 12, 2024 •

edited

Loading