-
Notifications
You must be signed in to change notification settings - Fork 224
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
Re-order and refactor help for client/server validation #1896
Re-order and refactor help for client/server validation #1896
Conversation
Put on my todo list for review/functionality test; but I can’t guarantee anything |
I think it’s not really used anywhere yet. The CLI page was and is created manually. |
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.
All in all it seems to be quite OK?
Ran "./Jamulus -e" which produced the error message:
./Jamulus: '--directoryserver' needs a string argument.
This might be confusing since we passed -e not --directoryserver. Also the error message with server only options might need some thoughts and tests (In some cases I can't remember "Did you omit --server" was confusing. Maybe ./Jamulus -e -c
?).
bb6709e
to
dbdcc4a
Compare
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.
Sorry for the delay - Jamulus is taking a back seat at the moment.
This PR looks ok to me. I agree with @ann0see's comment #1896 (review) regarding the error message. This could be addressed with the following diff:
diff --git a/src/main.cpp b/src/main.cpp
index 0a0a6fcb..e58d5e17 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -942,7 +942,7 @@ bool GetStringArgument ( int argc, char** argv, int& i, QString strShortOpt, QSt
{
if ( ++i >= argc )
{
- qCritical() << qUtf8Printable ( QString ( "%1: '%2' needs a string argument." ).arg ( argv[0] ).arg ( strLongOpt ) );
+ qCritical() << qUtf8Printable ( QString ( "%1: '%2' needs a string argument." ).arg ( argv[0] ).arg ( argv[i-1] ) );
exit ( 1 );
}
@@ -970,7 +970,7 @@ bool GetNumericArgument ( int argc,
QString errmsg = "%1: '%2' needs a numeric argument between '%3' and '%4'.";
if ( ++i >= argc )
{
- qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( strLongOpt ).arg ( rRangeStart ).arg ( rRangeStop ) );
+ qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i-1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
exit ( 1 );
}
@@ -978,7 +978,7 @@ bool GetNumericArgument ( int argc,
rValue = strtod ( argv[i], &p );
if ( *p || ( rValue < rRangeStart ) || ( rValue > rRangeStop ) )
{
- qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( strLongOpt ).arg ( rRangeStart ).arg ( rRangeStop ) );
+ qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i-1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
exit ( 1 );
}
Then the error message would mention the actual option supplied. But I don't feel strongly enough to block the PR for it.
dbdcc4a
to
733638b
Compare
Patched:
And again with format fixes. Bad Tony. Bad. ;) |
733638b
to
a81f107
Compare
Short description of changes
Review and refactor command line handling in
src/main.cpp
to standardise how it all works.Context: Fixes an issue?
Was part of #1867 (persistent directory lists) but really is nothing to do with it. Might unnecessarily cause conflicts in that change should
src/main.cpp
be amended by some other change, if left as part of it.There's what appears to be a lot of code change - in fact, it's mostly moving existing code around. The main changes are from (new) line 533 onwards.
Does this change need documentation? What needs to be documented and how?
The code change should be self-documenting, I hope.
This change affects the
--help
output of Jamulus, so any documetation that references that will need reviewing. I have not looked into that aspect.Status of this Pull Request
Working, tested code.
What is missing until this pull request can be merged?
Needs reviewing as an independent pull request. Had been reviewed under #1867 by @ann0see, I believe. See #1867 (review)
Checklist