Skip to content

Conversation

heckj
Copy link
Contributor

@heckj heckj commented Sep 20, 2024

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

/// Extended description of the command's functionality.
public var discussion: String?
/// Command should appear in help displays.
public var shouldDisplay: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add = true as a fall back for older json generated before this option was introduced?

Copy link
Contributor Author

@heckj heckj Oct 1, 2024

Choose a reason for hiding this comment

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

Updated now - apologies for the delay, didn't have my laptop with me for the past week. Updated and rebased to resolve conflicts.

Copy link
Contributor

@rauhul rauhul left a comment

Choose a reason for hiding this comment

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

small change needed, otherwise looks good

- resolves apple#668
  by extending the ToolInfoV0 (help dump) struct to include visibility
  information for commands (already exists for arguments).
- updates test output to verify existing examples extend with the
  additional key.
@heckj heckj force-pushed the dump-hidden-command branch from 4e8636c to 6652f22 Compare October 1, 2024 02:33
@heckj heckj requested a review from rauhul October 1, 2024 02:34
@rauhul
Copy link
Contributor

rauhul commented Oct 1, 2024

@swift-ci test

@heckj
Copy link
Contributor Author

heckj commented Oct 1, 2024

Thanks @rauhul - I missed one piece in the rebasing I think, just made a further commit to line that up properly, so next round should sail through CI without issue.. I hope. Locally verified, anyway.

@natecook1000
Copy link
Member

@swift-ci Please test

@rauhul
Copy link
Contributor

rauhul commented Oct 2, 2024

Sorry for the last minute nit pick, can we add a test case that results in false appearing in the json?

We can update struct C: ParsableCommand { to cover this case

@heckj
Copy link
Contributor Author

heckj commented Oct 2, 2024

@rauhul Sure, can definitely do that.

@heckj
Copy link
Contributor Author

heckj commented Oct 2, 2024

I've updated the existing test - command c now includes a configuration that "hides" it to illustrate the hiding in effect during dump. It changed the existing output rather than adding a new test case explicitly, so I think this covers what we're after, but if you want the same idea in it's own test case, I can revert the last commit and create a d command that illustrates a hidden command in the output.

@rauhul
Copy link
Contributor

rauhul commented Oct 2, 2024

ship it!

@rauhul
Copy link
Contributor

rauhul commented Oct 2, 2024

@swift-ci test

@natecook1000 natecook1000 merged commit 72bf212 into apple:main Oct 7, 2024
2 checks passed
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.

help-dump structures should reflect hidden attribute for commands as well as arguments
3 participants