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

Implement Subcommand Autodiscovery #319

Closed
wants to merge 4 commits into from

Conversation

Azoy
Copy link
Member

@Azoy Azoy commented May 24, 2021

This is an initial draft of subcommand auto discovery using reflection to find a list of protocol conformances to ParsableCommand and looking for nested types who conform to said protocol.

I haven't added any new tests, but I've changed some of the existing tests which essentially tests this functionality. There are still tests who have subcommands who aren't nested, thus they still use the subcommand array in CommandConfiguration which allows us to test that user defined subcommands also still work. If specific tests are needed, I'll be happy to add them! This is marked WIP because there are still some more documentation needed to fully explain this feature.

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

@Azoy Azoy requested a review from natecook1000 May 24, 2021 16:03
@Azoy
Copy link
Member Author

Azoy commented May 24, 2021

@swift-ci Please test

@Azoy Azoy force-pushed the auto-subcommand-discovery branch from d5419c0 to 8fe87ce Compare May 24, 2021 20:16
@Azoy
Copy link
Member Author

Azoy commented May 24, 2021

@swift-ci Please test

@kylemacomber
Copy link
Contributor

This is so cool!

@Azoy Azoy force-pushed the auto-subcommand-discovery branch from 8fe87ce to 8a6e82d Compare May 25, 2021 03:38
@Azoy
Copy link
Member Author

Azoy commented May 25, 2021

@swift-ci Please test

@Azoy
Copy link
Member Author

Azoy commented May 25, 2021

error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if _ptrauth(_arm64e)

Um, excuse me? (This has compiled in the past, and the stdlib makes use of this conditional as well??)

Edit: Seems the ci bot is using Swift 5.2.4 and this conditional wasn't released until 5.3

Copy link
Contributor

@kylemacomber kylemacomber left a comment

Choose a reason for hiding this comment

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

We should probably leave at least one test that explicitly lists out the subcommands and/or we should add a test that explicitly lists out the subcommands (and deliberately omits a nested ParsableCommand)

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.

This is looking great, @Azoy! 👏🏻👏🏻

In addition to the notes below, let's make sure we still have tests covering manually specified subcommands as well as the empty override (a command with nested subcommands, but with subcommands: [] as part of the configuration).

Sources/ArgumentParser/Utilities/SubcommandDiscovery.swift Outdated Show resolved Hide resolved
Sources/_Runtime/Metadata/ClassMetadata.swift Outdated Show resolved Hide resolved
Sources/_Runtime/Utilities/ImageInspection.swift Outdated Show resolved Hide resolved
Sources/_Runtime/Utilities/RelativePointers.swift Outdated Show resolved Hide resolved
@Azoy Azoy marked this pull request as ready for review May 30, 2021 22:53
@Azoy Azoy force-pushed the auto-subcommand-discovery branch from 03728e6 to 929bb38 Compare July 1, 2021 18:48
@hassila
Copy link

hassila commented Dec 7, 2021

@Azoy Did/would you consider splitting out _Runtime & _CRuntime to a separate GitHub SPM package? I'm starting to look at prototyping a dynamic loading plug-in API (https://forums.swift.org/t/swift-dynamic-loading-api/39495/21) and I think the approach you took there would be a possible nice way forward for that use case too. I was thinking of linking the plugins with that hypothetical separate package with the metadata runtime to allow for fully dynamic discovery of types conforming to the specific API protocol. It seems like generically interesting introspection capability...

@Azoy Azoy closed this Mar 5, 2023
@Azoy Azoy deleted the auto-subcommand-discovery branch March 5, 2023 23:02
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.

None yet

5 participants