Cleanup Argument parser. #3640

poelzi · 2021-02-17T16:38:56Z

Use QCommandLineParser instead of buggy hand crafted one.

Be-ing · 2021-02-17T17:18:39Z

Scanning dependencies of target mixxx-benchmark
[100%] Mixxx Benchmarks
Unknown option 'benchmark'.
CMakeFiles/mixxx-benchmark.dir/build.make:57: recipe for target 'CMakeFiles/mixxx-benchmark' failed
make[3]: *** [CMakeFiles/mixxx-benchmark] Error 1
CMakeFiles/Makefile2:1427: recipe for target 'CMakeFiles/mixxx-benchmark.dir/all' failed
CMakeFiles/Makefile2:1434: recipe for target 'CMakeFiles/mixxx-benchmark.dir/rule' failed
make[2]: *** [CMakeFiles/mixxx-benchmark.dir/all] Error 2
Makefile:730: recipe for target 'mixxx-benchmark' failed
make[1]: *** [CMakeFiles/mixxx-benchmark.dir/rule] Error 2
make: *** [mixxx-benchmark] Error 2
Error: Process completed with exit code 2.

???
seems unrelated to the changes in this branch... did Google Benchmark break?

poelzi · 2021-02-17T17:20:36Z

@Be-ing no, the problem was that I used process which will fail on unknown parameters and the benchmark will pass --benchmark. I changed that unknown parameters are not fatal anymore.

poelzi · 2021-02-17T17:29:43Z

@Be-ing the alternative would be to add --benchmark as a hidden parameter. This way unknown arguments would rise an error again. Personally, I don't care, we ignored unknown paremeters before, so the behaviour is like before.

poelzi · 2021-02-17T18:00:16Z

ready for review. There is one thing I could not fix, in the shell calling
"--settingsPath=~/blubb" does not work while "--settingsPath ~/blubb" does.
Using an absolute path always works, but in later case, the shell properly expands ~ in the first case it does not. Since I found no easy way to expand ~ in Qt, I accepted the case ;)

uklotzde

Nice cleanup. Changes look reasonable, have not tested it yet.

Any other opinions? Otherwise, please merge.

src/util/cmdlineargs.h

Holzhaus · 2021-02-22T22:13:23Z

src/util/cmdlineargs.cpp

+    parser.addOption(locale);
+
+    // An option with a value
+    const QCommandLineOption settingsPath(QStringLiteral("settingsPath"),


I'd love to get rid of these weird camelCase command line args and use names like --fullscreen and --settings-path instead. Out of scope for this PR though.

Suggested change

const QCommandLineOption settingsPath(QStringLiteral("settingsPath"),

const QCommandLineOption settingsPath(QStringLiteral("settingsPath"),

I added non camecase versions and added a comment that CamelCase arguments will be removed in 2.5

I'm okay with removing the camelCase versions for 2.4. I don't think there's any particular reason to delay that another release. What do others think?

Let's clean it up now.

Yes, I think it would be likely to be forgotten or procrastinated on finishing if we don't do it now.

src/util/cmdlineargs.cpp

…argparse

…up-argparse

Be-ing · 2021-03-03T07:00:24Z

src/util/cmdlineargs.cpp

+            "https://manual.mixxx.org/2.3/chapters/appendix.html#command-line-options).\n"
+            "CamelCase arguments are deprecated and will be removed in 2.5"));


Suggested change

"https://manual.mixxx.org/2.3/chapters/appendix.html#command-line-options).\n"

"CamelCase arguments are deprecated and will be removed in 2.5"));

"https://manual.mixxx.org/2.4/chapters/appendix.html#command-line-options"));

Please make a PR for the manual too.

Be-ing · 2021-03-03T07:01:40Z

Wait, are we removing the camelCase versions now or not?

poelzi · 2021-03-03T09:07:55Z

The reason I would like to have them stick around for a version is that I don't need 2 different argument versions when running 2.3 and 2.4 in parallel. I don't want to accidentally use the wrong settings folder all the time when testing a 2.3 branch.
2.3 and the version here does not exit on wrong arguments but silently ignores them.
Should I change it, that unknown arguments cause an error ? (then I need to add a hidden --benchmark argument for the tests).

uklotzde · 2021-03-03T09:27:32Z

The reason I would like to have them stick around for a version is that I don't need 2 different argument versions when running 2.3 and 2.4 in parallel.

Good point, I didn't consider this use case. Then a version with both variants available makes sense.

Holzhaus · 2021-03-03T09:44:15Z

2.3 and the version here does not exit on wrong arguments but silently ignores them.
Should I change it, that unknown arguments cause an error ? (then I need to add a hidden --benchmark argument for the tests).

Yes, I think showing an error is almost always preferable to silently ignoring them (except for special cases, e. g. during live use).

However, I fear it's not that simple because there are a lot more arguments that we'd need to handle because we use the argument parser for the tests binary, too. So we would need to add every single command line flag added by gtests. Therefore I think we should postpone that for another PR and maybe add a special argument parser that only supports setting the log level and does ignore unknown flags for tests only.

Be-ing · 2021-03-03T18:37:34Z

Looking at the code, the cost of keeping the camelCase variants is trivial; it is just adding another string literal to a QStringList. So I don't think there's any urgent reason to remove them.

Be-ing · 2021-03-03T18:42:58Z

The reason I would like to have them stick around for a version is that I don't need 2 different argument versions when running 2.3 and 2.4 in parallel.

Good point. This won't be an issue for switching between 2.4 and 2.5 because both will support the new style options. So I think we should stick with the original plan to keep the camelCase versions for 2.4 and remove them for 2.5.

Be-ing

If there are no objections, I'll merge this tomorrow.

Holzhaus · 2021-03-07T18:21:58Z

There's still an unresolved review comment from you above.

Holzhaus · 2021-03-09T11:34:16Z

There's still an unresolved review comment from you above.

I guess your review comment was irrelevant then?

Be-ing · 2021-03-09T11:54:23Z

It can be fixed in another PR.

daschuer · 2021-04-26T06:20:17Z

This messes up the --help console output
I have filed a bug: https://bugs.launchpad.net/mixxx/+bug/1926126

poelzi · 2021-04-26T08:28:50Z

@daschuer on which system type? Because I checked this on linux and had no such issue.

daschuer · 2021-04-26T09:28:43Z

I am on Ubuntu Focal

poelzi · 2021-04-26T09:50:58Z

Interesting. It looks like this here:

poelzi · 2021-04-26T09:52:52Z

Is it the argument list that moves the description so much to the right ?
Maybe a qt version difference ? I'm using qtbase-5.15.2

daschuer · 2021-04-26T10:46:50Z

I think it is because you are not using the terminal default width of 80.

poelzi · 2021-04-26T16:27:32Z

QT is pretty stupid when formatting the help text
https://github.com/qt/qtbase/blob/101581484bd278ce4a5f329ffb13c9e0b0e4c131/src/corelib/tools/qcommandlineparser.cpp#L1123

I will try splitting the obsolete versions into extra options without help, but I guess as long as we have multiple long versions, it will always look this bad :(

github-actions bot added the code quality label Feb 17, 2021

Cleanup Argument parser.

1fa364f

Use QCommandLineParser instead of buggy hand crafted one.

poelzi force-pushed the cleanup-argparse branch from 471771e to 1fa364f Compare February 17, 2021 16:40

Fix benchmark arguments

5b269b0

poelzi added 2 commits February 17, 2021 18:37

use const reference

232a6ff

use case insensitive log level matching

3be9387

uklotzde approved these changes Feb 22, 2021

View reviewed changes

src/util/cmdlineargs.h Outdated Show resolved Hide resolved

Holzhaus reviewed Feb 22, 2021

View reviewed changes

Holzhaus approved these changes Feb 22, 2021

View reviewed changes

Be-ing reviewed Feb 23, 2021

View reviewed changes

src/util/cmdlineargs.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Feb 23, 2021

View reviewed changes

src/util/cmdlineargs.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Feb 23, 2021

View reviewed changes

src/util/cmdlineargs.cpp Outdated Show resolved Hide resolved

poelzi and others added 6 commits February 27, 2021 23:10

rename to parse, add none camelcase parameters

11fa159

Merge branch 'main' of https://github.com/mixxxdj/mixxx into cleanup-…

382eb28

…argparse

CmdlineArgs: fix typo in comment

0889fb7

Merge branch 'main' of https://github.com/mixxxdj/mixxx into cleanup-…

5d33aa9

…argparse

fix wrong description

3543675

Merge branch 'cleanup-argparse' of github.com:poelzi/mixxx into clean…

49e6173

…up-argparse

Be-ing reviewed Mar 3, 2021

View reviewed changes

Be-ing approved these changes Mar 7, 2021

View reviewed changes

Be-ing merged commit e6ec561 into mixxxdj:main Mar 9, 2021

mixxxbot mentioned this pull request Aug 24, 2022

console help output is unreadable #10405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Argument parser. #3640

Cleanup Argument parser. #3640

poelzi commented Feb 17, 2021

Be-ing commented Feb 17, 2021

poelzi commented Feb 17, 2021

poelzi commented Feb 17, 2021

poelzi commented Feb 17, 2021

uklotzde left a comment

Holzhaus Feb 22, 2021 •

edited

Loading

poelzi Mar 1, 2021

Be-ing Mar 1, 2021

uklotzde Mar 1, 2021

Be-ing Mar 1, 2021

Be-ing Mar 3, 2021

Be-ing commented Mar 3, 2021

poelzi commented Mar 3, 2021

uklotzde commented Mar 3, 2021

Holzhaus commented Mar 3, 2021

Be-ing commented Mar 3, 2021

Be-ing commented Mar 3, 2021 •

edited

Loading

Be-ing left a comment

Holzhaus commented Mar 7, 2021

Holzhaus commented Mar 9, 2021 •

edited

Loading

Be-ing commented Mar 9, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

poelzi commented Apr 26, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

	const QCommandLineOption settingsPath(QStringLiteral("settingsPath"),
	const QCommandLineOption settingsPath(QStringLiteral("settingsPath"),

		"https://manual.mixxx.org/2.3/chapters/appendix.html#command-line-options).\n"
		"CamelCase arguments are deprecated and will be removed in 2.5"));

	"https://manual.mixxx.org/2.3/chapters/appendix.html#command-line-options).\n"
	"CamelCase arguments are deprecated and will be removed in 2.5"));
	"https://manual.mixxx.org/2.4/chapters/appendix.html#command-line-options"));

Cleanup Argument parser. #3640

Cleanup Argument parser. #3640

Conversation

poelzi commented Feb 17, 2021

Be-ing commented Feb 17, 2021

poelzi commented Feb 17, 2021

poelzi commented Feb 17, 2021

poelzi commented Feb 17, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

poelzi Mar 1, 2021

Choose a reason for hiding this comment

Be-ing Mar 1, 2021

Choose a reason for hiding this comment

uklotzde Mar 1, 2021

Choose a reason for hiding this comment

Be-ing Mar 1, 2021

Choose a reason for hiding this comment

Be-ing Mar 3, 2021

Choose a reason for hiding this comment

Be-ing commented Mar 3, 2021

poelzi commented Mar 3, 2021

uklotzde commented Mar 3, 2021

Holzhaus commented Mar 3, 2021

Be-ing commented Mar 3, 2021

Be-ing commented Mar 3, 2021 • edited Loading

Be-ing left a comment

Choose a reason for hiding this comment

Holzhaus commented Mar 7, 2021

Holzhaus commented Mar 9, 2021 • edited Loading

Be-ing commented Mar 9, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

poelzi commented Apr 26, 2021

daschuer commented Apr 26, 2021

poelzi commented Apr 26, 2021

Holzhaus Feb 22, 2021 •

edited

Loading

Be-ing commented Mar 3, 2021 •

edited

Loading

Holzhaus commented Mar 9, 2021 •

edited

Loading