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.

Sign up for GitHub

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

Jump to bottom

Improvements to --dump-help #335

Merged
merged 5 commits into from Aug 26, 2021
Merged

Improvements to --dump-help #335

merged 5 commits into from Aug 26, 2021

Conversation

rauhul
Copy link
Contributor

@rauhul rauhul commented Jul 8, 2021 •
edited

  • Removes HelpInfo in favor of a recursively defined CommandInfo
    which contains more raw metadata about the source command.
    Additionally, introduces a top level ToolInfo type with a
    serialization version to aid future tooling.

  • Updates tests to match the new serialized format.

  • Renames DumpHelpInfoGenerator to DumpHelpGenerator to align the
    type with the --dump-help flag.

@rauhul rauhul added the enhancement New feature or request label Jul 8, 2021
@rauhul rauhul requested a review from natecook1000 July 8, 2021 04:16
@rauhul rauhul marked this pull request as ready for review July 8, 2021 19:10
@rauhul
Copy link
Contributor Author

rauhul commented Jul 8, 2021

@swift-ci please test

@rauhul
Copy link
Contributor Author

rauhul commented Jul 8, 2021

@natecook1000 what are your thoughts on the serialization format changes? I'm also wondering if the format should be evolvable/versioned or expect all clients to stay in lock step with the format? I defaulted towards the former by introducing a serializationVersion property.

@rauhul
Copy link
Contributor Author

rauhul commented Jul 13, 2021

@swift-ci please test

natecook1000
natecook1000 reviewed Jul 19, 2021
View reviewed changes
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.

Thanks for this, @rauhul! I think the changes make sense, esp along with the serialization version, since I know there will be additional features coming that can't be captured currently (bc they don't exist). Added a variety of notes, this largely looks ready to go.

FYI I think I'm going to limit access to the API and change the flag name to --experimental-dump-help in the first release of this.

Sources/ArgumentParser/Usage/ToolInfo.swift Outdated Show resolved Hide resolved
Sources/ArgumentParser/Usage/ToolInfo.swift Outdated Show resolved Hide resolved
Sources/ArgumentParser/Usage/ToolInfo.swift Outdated Show resolved Hide resolved
Sources/ArgumentParser/Usage/ToolInfo.swift Outdated Show resolved Hide resolved
Sources/ArgumentParser/Usage/ToolInfo.swift Outdated Show resolved Hide resolved
@rauhul
Copy link
Contributor Author

rauhul commented Jul 21, 2021

@swift-ci please test

@natecook1000
Copy link
Member

@swift-ci Please test

@rauhul
Copy link
Contributor Author

rauhul commented Jul 22, 2021

@natecook1000 It looks like the linux build is failing due to the use of @_spi, and the macOS build is failing due to the use of XCTExpectFailure. Any thoughts on how to proceed?

@natecook1000
Copy link
Member

Ugh — looking at whether we can use a higher version on CI to support this.

@rauhul
Copy link
Contributor Author

rauhul commented Jul 27, 2021

Would it be possible to only bump the linux version? Since swift is not ABI stable on linux, I think users have no good reason to use old versions on linux.

The macOS failure will be resolved by the other issue 340 PR

@rauhul
Improvements to --dump-help
291fc0a 
- Removes `HelpInfo` in favor of a recursively defined `CommandInfo`
  which contains more raw metadata about the source command.
  Additionally, introduces a top level `ToolInfo` type with a
  serialization version to aid future tooling.

- Updates tests to match the new serialized format.

- Renames `DumpHelpInfoGenerator` to `DumpHelpGenerator` to align the
  type with the `--dump-help` flag.
@rauhul
Address PR review comments
61bc75a 
- Renames `optional` to `nonEmpty`.

- Adds `allValues` field to `ArgumentInfoV0`.

- Adds test covering Issue #340 failure.
@rauhul
Rebase on main
3243904
@rauhul
Copy link
Contributor Author

rauhul commented Jul 29, 2021

@swift-ci please test

1 similar comment
@rauhul
Copy link
Contributor Author

rauhul commented Jul 29, 2021

@swift-ci please test

@rauhul
Copy link
Contributor Author

rauhul commented Jul 30, 2021

@swift-ci please test

@rauhul
Copy link
Contributor Author

rauhul commented Aug 4, 2021 •
edited

@compnerd how do you build with cmake? If you don't mind sending me a patch, I'd be happy to include whatever changes you'd like for the cmake build.

@compnerd
Copy link
Collaborator

compnerd commented Aug 4, 2021

You should be able to install a windows snapshot toolchain and then just use cmake -B out -G Ninja -S . -DCMAKE_BUILD_TYPE=Release && ninja -C build. I'll try to see if I can find some time to test this in the next few days if you've not beat me to it.

@natecook1000 natecook1000 added this to the ArgumentParser 1.0 milestone Aug 26, 2021
@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit b3bef58 into apple:main Aug 26, 2021
@compnerd
Copy link
Collaborator

This seems to have regressed the CMake build :-(

compnerd added a commit to compnerd/swift-argument-parser that referenced this pull request Aug 30, 2021
@compnerd
build: repair the build of ArgumentParser with CMake
a1c47f3 
The changes in apple#335 introduced a new library but failed to actually
build the library and update the dependency structure.  Update the build
system to build and install the new target.
@compnerd compnerd mentioned this pull request Aug 30, 2021
Merged
4 tasks
natecook1000 pushed a commit that referenced this pull request Aug 30, 2021
@compnerd
build: repair the build of ArgumentParser with CMake (#349)
eef1f75 
The changes in #335 introduced a new library but failed to actually
build the library and update the dependency structure.  Update the build
system to build and install the new target.
@rauhul rauhul deleted the rauhul/improve-dump-help branch August 31, 2021 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natecook1000 natecook1000 natecook1000 left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
ArgumentParser 1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rauhul @natecook1000 @compnerd