Skip to content
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

Simplify synopsis string generation #316

Merged
merged 2 commits into from
May 22, 2021
Merged

Simplify synopsis string generation #316

merged 2 commits into from
May 22, 2021

Conversation

rauhul
Copy link
Contributor

@rauhul rauhul commented May 20, 2021

  • Removes unused codepaths.
  • Simplifies synopsis string codepaths by removing optionality. This
    complexity is moved to the caller who is now responsible for filtering
    out hidden arguments and options. This change is desirable as it
    allows the caller to determine if the argument should be hidden. For
    example, while it makes sense to hide arguments in help text, it may
    not make sense to hide them when dumping the arguments for another
    tool to consume.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@rauhul rauhul requested a review from natecook1000 May 20, 2021 16:47
@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@swift-ci please test

@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@natecook1000 I recommend hiding whitespace changes when viewing this diff

@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@swift-ci please test

2 similar comments
@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@swift-ci please test

@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@swift-ci please test

@natecook1000
Copy link
Member

Thanks, @rauhul! Do you mind dropping the whitespace changes? I'd rather not introduce conflicts with other things that are in flight.

@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@natecook1000 yep I can drop the whitespace changes!

@rauhul
Copy link
Contributor Author

rauhul commented May 20, 2021

@swift-ci please test

@@ -371,7 +371,7 @@ extension HelpGenerationTests {
USAGE: l [--remote <remote>]

OPTIONS:
-t, -x, -y, --remote, --when, --time, -other, --there <remote>
-t, -x, -y, -other, --remote, --there, --time, --when <remote>
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, other than moving short names to the beginning, the old version preserved the declaration order of the names, which seems important for at least some cases. What’s motivating this change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a fair point, I thought it made more sense to put the arguments in order, but you're right, using declaration order allows the user to move depreciated flags to the back of the help, I'll back out this change.

@rauhul rauhul changed the title General gardening Simplify synopsis string generation May 21, 2021
@rauhul
Copy link
Contributor Author

rauhul commented May 21, 2021

@swift-ci please test

- Removes unused codepaths.
- Simplifies synopsis string codepaths by removing optionality. This
  complexity is moved to the caller who is now responsible for filtering
  out hidden arguments and options. This change is desirable as it
  allows the caller to determine if the argument should be hidden. For
  example, while it makes sense to hide arguments in help text, it may
  not make sense to hide them when dumping the arguments for another
  tool to consume.
@rauhul
Copy link
Contributor Author

rauhul commented May 21, 2021

@swift-ci please test

@@ -36,49 +36,48 @@ extension UsageGenerator {
///
/// In `roff`.
var synopsis: String {
let definitionSynopsis = definition.synopsis
switch definitionSynopsis.count {
// Filter out options that should not be displayed.
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch 👍🏻

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit f4353db into apple:main May 22, 2021
@rauhul rauhul deleted the rauhul/gardening branch May 24, 2021 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants