Skip to content

[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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kwk
Copy link
Contributor

@kwk kwk commented Jun 2, 2025

This is a fixup for #141851 and removes = from all
options with additional arguments.

Before 14 out of 22 options with arguments used "=" and 7 didn't.

@kwk kwk requested a review from RoboTux June 2, 2025 07:20
@kwk kwk self-assigned this Jun 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 2, 2025

@llvm/pr-subscribers-testing-tools

Author: Konrad Kleine (kwk)

Changes

This is a fixup for #141851.


Full diff: https://github.com/llvm/llvm-project/pull/142340.diff

1 Files Affected:

  • (modified) llvm/docs/CommandGuide/lit.rst (+1-1)
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

Copy link
Contributor

@nikic nikic left a 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?

@kwk kwk changed the title [docs] use = in --max-retries-per-test=N [docs] use "=" in options with arguments Jun 2, 2025
@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

It seems like the docs use a mix of both styles -- should we normalize to one or the other for all options?

Oh, indeed. I must say in terms of readability I like the one without the = better.

Here are some stats:

  • There are 53 options in the file
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | wc -l
  • There are 22 options with additional arguments (e.g. in capital letters)
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z=]' | wc -l
  • Out of the 22 options with arguments, there are 14 options that use "=" and 7 that use no "=".
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z]' | grep "=" | wc -l
    • grep '.. option::' llvm/docs/CommandGuide/lit.rst | grep -P '[A-Z]' | grep -v "=" | wc -l

That means the majority of options with additional arguments uses =.

Consistency wise we should probably use = for this PR and the rest as well. I've changed this PR accordingly.

@kwk kwk force-pushed the fixup-for-141851 branch from f72b7a8 to 9432d15 Compare June 2, 2025 10:41
Copy link
Contributor

@nikic nikic left a 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...

@@ -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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does.

@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

The print test that lit itself prints uses whitespace instead of =, so I'd be inclined to normalize the other way around...

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.
@kwk kwk force-pushed the fixup-for-141851 branch from 9432d15 to aa68d4e Compare June 2, 2025 15:37
@kwk kwk changed the title [docs] use "=" in options with arguments [docs] don't use "=" in lit options with arguments Jun 2, 2025
@kwk
Copy link
Contributor Author

kwk commented Jun 2, 2025

@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]
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants