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

Add new built-in flag --dump-help #310

Merged
merged 31 commits into from
Jul 7, 2021
Merged

Conversation

KS1019
Copy link
Contributor

@KS1019 KS1019 commented May 17, 2021

This PR resolves #164 and adds a new built-in flag --dump-help to dump all the help information in JSON format. This feature can be useful when parsing help information and generating documentation for a command. I will add documentation if necessary. In that case, I would appreciate it if anyone could direct me to the right place to add the documentation.

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

@KS1019
Copy link
Contributor Author

KS1019 commented May 17, 2021

@natecook1000 Please take a look when you have time. Thank you!

Edit: I did not realize the conflicts when I made PR and I will need a little more time to resolve them so I am making this a Draft PR.

I have resolved the conflicts so please take a look. Thank you!

@KS1019 KS1019 marked this pull request as draft May 17, 2021 10:16
@KS1019 KS1019 marked this pull request as ready for review May 17, 2021 11:16
Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Looks like a good start, @KS1019! I haven't gotten a chance to review DumpHelpInfoGenerator yet, but wanted to give you a couple notes here.

Comment on lines 84 to 87
// Look for `--dump-help`
guard !split.contains(Name.long("dump-help")) else {
throw DumpHelpInfoRequested()
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather handle this the same way as --version rather than than going through than separate DumpHelpInfoCommand command. HelpCommand exists so that you can write things like example help sub1 sub2, but that won't be the case here, it's just a short-circuiting flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At 4d97d0c, I modified the code to throw CommandError like --version does. I will look into more if I can remove DumpHelpInfoCommand completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed DumpHelpInfoRequested and DumpHelpInfoCommand at 9825ecb

@@ -185,6 +185,7 @@ internal struct HelpGenerator {
optionElements.append(element)
}
}
optionElements.append(.init(label: "--dump-help", abstract: "Dump help information."))
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this out of the help screen for now. I'd like to eventually add a version of the help screen that includes hidden flags, etc (via something like example --help --all or example --help-all), so this could show up as part of that.

Copy link
Contributor Author

@KS1019 KS1019 May 20, 2021

Choose a reason for hiding this comment

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

Addressed at 40de584

@_implementationOnly import Foundation

internal struct DumpHelpInfoGenerator {
struct CommandInfo: Codable {
Copy link
Member

Choose a reason for hiding this comment

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

I think CommandInfo and ArgumentInfo will probably be independently useful — can you move them out to be top-level types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed at 94a81ed

var isDefault: Bool?
}

var command: ArgumentInfo
Copy link
Member

Choose a reason for hiding this comment

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

Let's not re-use this for the command's metadata — should be okay to just put the command-specific details right here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 9c8e5a7

@natecook1000
Copy link
Member

@swift-ci Please test

@KS1019
Copy link
Contributor Author

KS1019 commented Jun 23, 2021

Hi @natecook1000
I fixed codes that had compilation errors and made changes to follow the upstream changes. Please take a look again when you have time!

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000
Copy link
Member

@swift-ci Please test Linux platform

@KS1019
Copy link
Contributor Author

KS1019 commented Jul 1, 2021

Hi @natecook1000

It seems that tests are failing on the Linux Platform because specifying only encoder.outputFormatting = .prettyPrinted gives me same JSON but in different order depending on platforms.

I am considering adding .sortedKeys to encoder.outputFormatting but since they are available only on macOS 10.13 or newer, I am not sure if that's the best option.

Alternatively, I think we can change how we test the generated JSON from comparing them as strings to comparing them as values by converting them back to the codable values.

I would love to know what do your think the best way would be to solve the problem!

@natecook1000
Copy link
Member

@KS1019 I think it would be fine to round trip through JSON and check for equality with the expectation that way. String equality for JSON is definitely tricky due to its order independence. Thanks!

@KS1019
Copy link
Contributor Author

KS1019 commented Jul 3, 2021

@natecook1000
I've made changes to address the issues with testing JSONs. Please take a look when you have time!

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit cfcb9cb into apple:main Jul 7, 2021
@KS1019 KS1019 deleted the dump-help-info branch July 8, 2021 06:50
compnerd added a commit to compnerd/swift-argument-parser that referenced this pull request Jul 8, 2021
Repair the build after apple#310 to allow building with CMake which is required for bootstrapping.
@compnerd
Copy link
Collaborator

compnerd commented Jul 8, 2021

This regressed the Windows build :-( (#336 should repair that)

natecook1000 pushed a commit that referenced this pull request Jul 9, 2021
Repair the build after #310 to allow building with CMake which is required for bootstrapping.
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.

Provide a way to dump the help text for all commands and options in JSON
3 participants