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 hide check to write_arg_markdown #15

Closed
wants to merge 6 commits into from

Conversation

wackazong
Copy link

Hidden arguments are now correctly filtered out from the markdown documentation.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 28, 2024

This PR includes a lot of unrelated changes. I think only the snippet

    // Don't document commands marked with `clap(hide = true)`.
    if arg.is_hide_set() {
        return Ok(());
    }

is relevant.

I am not the maintainer, but I think that, to make this PR mergeable, you should get rid of all the unrelated stuff and change one of the tests to have a hidden argument.

(I do like many of your unrelated changes, but you could make them a separate draft PR or you could post a link to your fork with those changes).

@ConnorGray , I'm happy to create a cleaner PR with a test myself, if it helps and if you are planning/able to release any future versions of this library. (Also, thanks for writing this library!)

@wackazong
Copy link
Author

@ilyagr your are absolutely right, I accidentally pushed more changes to this PR. Only the first commit is relevant. And I agree with you regarding the test as well. Thanks for the input.

@ilyagr
Copy link
Contributor

ilyagr commented Jun 13, 2024

I put a cleaned-up version of this with tests in #22, specifically 48907e1. My approach is a little different, note the test of a command that only has hidden options.

@ConnorGray
Copy link
Owner

@wackazong @ilyagr Thank you both for your contributions to this library, and please accept my apologies for the delay in processing PRs in this repo😌

I agree with @ilyagr this is a bit large to merge in one go, but I've merged the piece Ilya broke out in #22; if there are other parts of this PR you'd like to see get merged, I'm happy to discuss them in a new PR 🙂

@ConnorGray ConnorGray closed this Jun 15, 2024
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.

3 participants