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

Try to improve CLI parsing #8

Closed
wants to merge 21 commits into from
Closed

Try to improve CLI parsing #8

wants to merge 21 commits into from

Conversation

@cosmo-ray
Copy link
Contributor

@cosmo-ray cosmo-ray commented Dec 30, 2016

Try to fix this:
https://wiki.xoreos.org/index.php?title=TODO#Command_line_parsing_in_the_tools
I've try to keep backward compatibility, so the usage should be just the same as before, I was unable to test every options of every program, but the one I've try was working, I hope I've break nothing.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Dec 30, 2016

Neat, thanks! :)

I probably won't have time to look at it in detail before January 1st, though.

One minor thing I can see immediately: the commit message should begin with an uppercase letter after the module tag. I.e. "COMMON: Add a new CLI class" instead of "COMMON: add a new CLI class".

@cosmo-ray cosmo-ray force-pushed the cosmo-ray:cli branch from 306416d to f1b8470 Jan 1, 2017
@cosmo-ray
Copy link
Contributor Author

@cosmo-ray cosmo-ray commented Jan 3, 2017

commit messages are now fix

@cosmo-ray
Copy link
Contributor Author

@cosmo-ray cosmo-ray commented Feb 23, 2017

ping

Copy link
Member

@DrMcCoy DrMcCoy left a comment

I'm really sorry, but didn't find any time for this earlier.

In principle, I like your implementation, good work! :)

There are a lot of style-issues, though, most prominently a lot of trailing whitespace (in more than just the places I marked). And the Callbacks leak. It's nothing major, but if you could fix those, that would be great.

I'm also not too sure I like the --help text output that much. In the manual ones, I introduced some groupings via newlines and I also hand-wrapped a few strings that overflowed 80 characters. If you know of a neat way to do that for your automatic text as well, I'd appreciate it.

helpStr += " ";
helpStr += optionArgs;
for (int i = (maxArgLength - strlen(longName) - strlen(optionArgs) - 4); i > 0; --i) {
helpStr += " ";

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

_val.insert(args[j]);
return j - i;
}

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

}
}

void Parser::addOption(const char *longName, char shortName, const char *help, OptionRet ret,

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

fail:
_returnVal = 1;
this->printUsage(_helpStr);
return false;

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

_print(aPrinter),
_printerStr(str) {}


This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

NoOption inFileOpt(false, new ValGetter<Common::UString &>(inFile, "input files"));
NoOption outFileOpt(true, new ValGetter<Common::UString &>(outFile, "output files"));
Parser parser(argv[0], "BioWare TLK to XML converter",
"If no output file is given, the output is written to stdout.\n\n"

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Add a \n at the start of that string too, please, and for the explanatory texts in other tools too.

std::fprintf(stream, "column layout. They will be pasted together and printed as one GDA.\n\n");
std::fprintf(stream, "If no output file is given, the output is written to stdout.\n");
}
// std::fprintf(stream, " -o <file> --output <file> Write the output to this file\n");

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Any reason you kept this?

"If no ID is given is given, it is guessed from the file name.",
returnValue, makeEndArgs(&inFileOpt, &outFileOpt));


This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Trailing whitespace

parser.addOption("dragonage2", "Use Dragon Age II encodings", kContinueParsing,
makeAssigners(new ValAssigner<GameID>(Aurora::kGameIDDragonAge2, game)));
parser.addOption("encoding", "Override an encoding", kContinueParsing,
new Callback<EncodingOverrides &>("str", parseEncodingOverride, encOverrides));

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

valgrind says that this line leaks, the new Callback

makeAssigners(new ValAssigner<GameID>(Aurora::kGameIDJade, game)));
parser.addOption("pass", "Decryption password, if required, in hex notation",
kContinueParsing,
new Callback<std::vector<byte> &>("hex", parsePassword, password));

This comment has been minimized.

@DrMcCoy

DrMcCoy Feb 24, 2017
Member

Here too the Callback (and in the lines below) leak

@cosmo-ray cosmo-ray force-pushed the cosmo-ray:cli branch 2 times, most recently from e543567 to bb16123 Mar 4, 2017
@cosmo-ray cosmo-ray force-pushed the cosmo-ray:cli branch from bb16123 to 5c99cd9 Mar 30, 2017
Matthias Gatto and others added 20 commits Oct 10, 2016
The goal of this new class is to simplifie the arguments parsing,
I've take inspiration from glib Commandline option parser:
https://developer.gnome.org/glib/stable/glib-Commandline-option-parser.html,
exept that with glib you need to specifie with a flag the type of each options,
here options type are automatically determine when adding an option
with parser.addOption.

Basically most of the job is made in the parser contructor, here you can
set the name of the program, summary, and arguments that are not options.

Option like "--nwm" are set with addOption function.
"--help" is automatically generated with arguments you've use in
addOption and in the contructor, "--version" is generated too, and
call Version::printVersion.
In this file, "Usage: xml2ssf [<options>] [<input file>] <output file>"
change to
"Usage: ./bin/xml2ssf [<options>] <[input file] <output file>>".

I had to use an array of stings to get input file and output file,
this is because input file is the optinal argument and is before ouput file,
I would be harder to handle this case automatically in cli.h, as I would need to
check if there is non optinal argument after the optinal one.

I could do that, but as this file is the only one that need that, I don't think
it worth the effort so I just get all arguments in a vector,
and let some bytes of the old parser do what's left of the job
@cosmo-ray cosmo-ray force-pushed the cosmo-ray:cli branch from 5c99cd9 to effc78e Apr 12, 2017
Signed-off-by: Matthias Gatto <uso.cosmo.ray@gmail.com>
@cosmo-ray
Copy link
Contributor Author

@cosmo-ray cosmo-ray commented Apr 13, 2017

thanks for the review and sorry for the time I take to answer.
I've try to fix all issues, I add a Parse::addSpace() method to handle groupings via newlines,
I didn't add "\n" in any explanatory texts, but I've made Parser::process automatically prepend a "\n" to the explanatory text if the string isn't empty.

@DrMcCoy
Copy link
Member

@DrMcCoy DrMcCoy commented Apr 13, 2017

Very nice, thank you! :)

I merged it with this push: f0dc347...1b6d66a.

@DrMcCoy DrMcCoy closed this Apr 13, 2017
DrMcCoy pushed a commit to xoreos/xoreos-wiki that referenced this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.