Re-order and refactor help for client/server validation#1896
Re-order and refactor help for client/server validation#1896pljones merged 1 commit intojamulussoftware:masterfrom
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.
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?).
pljones
requested review from
a team,
gilgongo and
hoffie
and removed request for
a team and
hoffie
pljones
force-pushed
the
refactor-jamulus-cli-handling
branch
from
bb6709e to
dbdcc4a
Compare
softins
left a comment
softins
left a comment
There was a problem hiding this comment.
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.
pljones
force-pushed
the
refactor-jamulus-cli-handling
branch
from
dbdcc4a to
733638b
Compare
|
Patched: And again with format fixes. Bad Tony. Bad. ;) |
pljones
force-pushed
the
refactor-jamulus-cli-handling
branch
from
733638b to
a81f107
Compare
3 participants
Short description of changes
Review and refactor command line handling in
src/main.cppto 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.cppbe 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
--helpoutput 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