-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cmdlineargs: Error handling and translation #4170
Conversation
after the QApplication has processed the command line as well. This allows proper feedback when a unknown command line option is found and considers also the Qt internal command line arguments. As extra bounty the help output is now translated.
993f365
to
e951cd5
Compare
I don't understand what "user feedback" means in this context. |
Pull Request Test Coverage Report for Build 1096626455
💛 - Coveralls |
Main silently starts if you have a typo in the command line option. Now it looks like this:
|
The messages before the new line are slipping through before the logging is configured. |
src/util/cmdlineargs.cpp
Outdated
|
||
void CmdlineArgs::parseForUserFeedback() { | ||
DEBUG_ASSERT(QCoreApplication::instance()); | ||
if (!m_hasUserFeedback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition seems to be wrong or I misunderstand the purpose/naming of the boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need only execute the second run when the first run has produced user feedback, otherwise we can skip the second run. I am happy to rename the variable if you have a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_requiresUserFeedback
or m_needsUserFeedback
? German: "erfordert"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be read as if we require feedback from the user.
How about m_parseForUserFeedbackRequired?
src/util/cmdlineargs.cpp
Outdated
|
||
bool CmdlineArgs::parse(const QStringList& arguments, bool forUserFeedback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add enum class ParseMode
instead of using a boolean.
Done. |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Fails fast with unrecognized command line options and doesn't let them slip through silently. LGTM
Added a second parser path for user feedback after the QApplication has processed the command line as well.
This allows proper feedback when a unknown command line option is found and considers also the Qt internal command line arguments.
As extra bounty the help output is now translated.