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

1.7 - Better arg parsing #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bradenbest
Copy link

@bradenbest bradenbest commented May 26, 2024

  • Heavily refactored arg parsing to use a simple search-and-dispatch pattern
  • The argument order no longer matters. The last filename entered is the one used.
  • longopts can now take arguments in --key=value or --key value format
  • Added -c to --color
  • -h/--help is now a canon option
  • -- now disables opt parsing instead of merely exiting the arg parsing loop
  • updated man page to explain what -s/--sector does
  • made an effort to keep code style as similar to pixel's as possible (next line brace, 2-space indent)

Feel free to review my changes. I'm not used to automake/m4, but I think I got everything. I even logged my changes in the changelog, which I've duplicated above (that's just how I format my commits).

I made an effort to keep the style as similar to yours as possible. I observed camelCase, 2-space indent and next line braces. I did break the rules in a couple places, namely parseArg_matchOpt. The underscores are a "pseudo namespace". I think it's reasonable. I also added one type, ParseArgCallbackFn, in PascalCase because I assume that's the convention (PascalCase for types, camelCase for functions and variables).

I tried to keep the changes small in scope, which is why the LUT in parseArg is an anonymous struct and why there aren't any enums, like for the return value of parseArg_matchOpt.

The new arg parser loop doesn't behave completely identically to the old one, but the changes I made are sane and reasonable. For example, the order no longer matters, you can do filename -s or -s filename. There was also some stipulation about throwing the usage if argc > 1 after the arg parser loop is done. I did away with that. I was careful to preserve the behavior when fileName is NULL: the program will make one attempt to obtain it interactively (findFile), and then either exit with an error message or open the file.

That's about all that comes to mind. Nice app!
-Braden Best (bradenbest)

edit: fixes #70

- Heavily refactored arg parsing to use a simple search-and-dispatch pattern
- The argument order no longer matters. The last filename entered is the one used.
- longopts can now take arguments in --key=value or --key value format
- Added -c to --color
- -h/--help is now a canon option
- -- now disables opt parsing instead of merely exiting the arg parsing loop
- updated man page to explain what -s/--sector does
- made an effort to keep code style as similar to pixel's as possible (next line brace, 2-space indent)

Feel free to review my changes. I'm not used to automake/m4, but I think
I got everything. I even logged my changes in the changelog, which I've
duplicated above (that's just how I format my commits).

I made an effort to keep the style as similar to yours as possible. I
observed camelCase, 2-space indent and next line braces. I did break the
rules in a couple places, namely parseArg_matchOpt. The underscores are
a "pseudo namespace". I think it's reasonable. I also added one type,
ParseArgCallbackFn, in PascalCase because I assume that's the convention
(PascalCase for types, camelCase for functions and variables).

I tried to keep the changes small in scope, which is why the LUT in
parseArg is an anonymous struct and why there aren't any enums, like for
the return value of parseArg_matchOpt.

The new arg parser loop doesn't behave completely identically to the old
one, but the changes I made are sane and reasonable. For example, the
order no longer matters, you can do `filename -s` or `-s filename`.
There was also some stipulation about throwing the usage if argc > 1
after the arg parser loop is done. I did away with that. I was careful
to preserve the behavior when fileName is NULL: the program will make
one attempt to obtain it interactively (findFile), and then either exit
with an error message or open the file.

That's about all that comes to mind. Nice app!
-Braden Best (bradenbest)
@bradenbest
Copy link
Author

Oh, I forgot to include this in the commit: fixes #70

@bradenbest
Copy link
Author

bradenbest commented May 26, 2024

okay, not sure what happened to the manpage edit. The changes were basically:

  • --color: added -c
  • --sector: added a sentence below saying something like "groups bytes in groups of 8, ignores --linelength and sets linelength to 16"
  • changed one instance of "1.6" to "1.7"

The fact that the 1.7 is the only change that persists tells me that it's autogenerated by the build system. So that's one thing I messed up. Alas, I'm not sure what to change to make it stick.

Here's what the manpage on my system looks like after I manually edited it:
image

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.

Commandline parsing should be as order-agnostic as reasonably possible
1 participant