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

ArgParser rewrite #3946

Merged
merged 5 commits into from
Sep 8, 2024
Merged

ArgParser rewrite #3946

merged 5 commits into from
Sep 8, 2024

Conversation

YoshiRulz
Copy link
Member

@YoshiRulz YoshiRulz commented Jun 11, 2024

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

Don't merge without checking the commit hash in README.md (it should be of the commit immediately before the readme change).

@Morilli
Copy link
Collaborator

Morilli commented Jun 12, 2024

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

@YoshiRulz
Copy link
Member Author

YoshiRulz commented Jun 12, 2024

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
Copy link
Member Author

YoshiRulz commented Jul 4, 2024

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. Thinking about this again, I'm always annoyed when I run appname --help and it's not in alphabetical order (youtube-dl), so I'm now decidedly against that.

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

@Morilli Morilli left a comment

Choose a reason for hiding this comment

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

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).

src/BizHawk.Client.Common/ArgParser.cs Show resolved Hide resolved
README.md Outdated
@@ -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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@YoshiRulz YoshiRulz Sep 5, 2024

Choose a reason for hiding this comment

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

I've fixed this by caveating it with "on Linux".

(The CommandLineBuilder.UseHelp(Action<HelpContext>) overload does seem to run on-demand i.e. only when --help is passed, if you wanted to look into that Win32 Console hack post-merge.)

src/BizHawk.Client.Common/ArgParser.cs Show resolved Hide resolved
src/BizHawk.Client.Common/ArgParser.cs Show resolved Hide resolved
@YoshiRulz
Copy link
Member Author

YoshiRulz commented Sep 5, 2024

Rebased, only differences are:

  • readme edits now reflect --help being Linux-only, and that the next release will be 2.10; and
  • NuGet dep was made non-propagating. Apparently this stopped the assembly from being copied to the output. Fixed with 09f0375.

There are no actionable review comments left (edit: for me to do, that is; the possibility for --help on Windows remains), so I'll leave about a day for final comments before merging this.
(I'd like this in 2.10-RC1 to give the greatest chance of catching regressions. Yesterday on Discord we discussed getting to an RC soon-ish, which I believe we can do!)

@YoshiRulz YoshiRulz merged commit 9c93cc0 into master Sep 8, 2024
8 checks passed
@YoshiRulz YoshiRulz deleted the cli branch September 8, 2024 09:48
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