-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[docs] don't use "=" in lit options with arguments #142340
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-testing-tools Author: Konrad Kleine (kwk) ChangesThis is a fixup for #141851. Full diff: https://github.com/llvm/llvm-project/pull/142340.diff 1 Files Affected:
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 86879f870e06e..7f2ea6b82a716 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -218,7 +218,7 @@ EXECUTION OPTIONS
Stop execution after the given number of failures.
-.. option:: --max-retries-per-test N
+.. option:: --max-retries-per-test=N
Retry running failed tests at most ``N`` times.
Out of the following options to rerun failed tests the
|
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.
It seems like the docs use a mix of both styles -- should we normalize to one or the other for all options?
=
in --max-retries-per-test=N
Oh, indeed. I must say in terms of readability I like the one without the Here are some stats:
That means the majority of options with additional arguments uses Consistency wise we should probably use |
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.
The print text that lit
itself prints uses whitespace instead of =
, so I'd be inclined to normalize the other way around...
llvm/docs/CommandGuide/lit.rst
Outdated
@@ -112,7 +112,7 @@ OUTPUT OPTIONS | |||
|
|||
Enable -v, but for all tests not just failed tests. | |||
|
|||
.. option:: -o PATH, --output PATH | |||
.. option:: -o=PATH, --output=PATH |
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.
Hm, -o=PATH
looks quite unusual.
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.
It does.
Like I said, this would have been my favourable way too but I went with the majority of what was in the file. Let me work on this real quick. |
This is a fixup for llvm#141851 and removes `=` from all options with additional arguments. Before 14 out of 22 options with arguments used "=" and 7 didn't.
@nikic can you recheck? |
|
||
Search for :file:`{NAME}.cfg` and :file:`{NAME}.site.cfg` when searching for | ||
test suites, instead of :file:`lit.cfg` and :file:`lit.site.cfg`. | ||
|
||
.. option:: -D NAME[=VALUE], --param NAME[=VALUE] | ||
.. option:: -D NAME [VALUE], --param NAME [VALUE] |
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.
Is this change correct? I think =
here is part of the parameter argument.
This is a fixup for #141851 and removes
=
from alloptions with additional arguments.
Before 14 out of 22 options with arguments used "=" and 7 didn't.