praiskup / argparse-manpage

Automatically build man-pages for your Python project
Apache License 2.0
40 stars 21 forks source link

Add --include command-line flag #80

Closed rrthomas closed 1 year ago

rrthomas commented 1 year ago

I have tried to mimic as best I can the behaviour of help2man, without changing the way that argparse-manpage works already.

The main effect of this is that the treatment of certain sections that are individually rendered by argparse-manpage is slightly different from help2man.

I have also renamed the non-standard “AUTHORS” section to the standard “AUTHOR”.

I have not yet tried to document or add tests for the new functionality, in case it’s desired to change it before merging.

praiskup commented 1 year ago

Thank you very much for your contribution!

Can you please fix the old tests? Can you please add a new one (or two) for your use-case? I'd like not to break your use-case in the future.

praiskup commented 1 year ago

Having some docs for the new --include feature in README.md would be awesome too.

rrthomas commented 1 year ago

I have attempted to fix the lint and test failures.

I'm not sure quite what's happening with the tests: if I run the supplied check script, they all pass. However, if I run tox, then some of the tests fail.

I had a look at the test results, changing tox.ini so it ran pytest with -vv so I could see the full diffs, and there were some differences. I think I fixed some of the problems by updating the date stamps in the expected output.

The remaining tests pass with tox if I change the width of my terminal. argparse-manpage should probably set COLUMNS to a fixed number (I use 999 with help2man) before running the parser with --help.

I still can't work out how ./check was passing; I tried upgrading my local pytest to the latest 7.3.0 so it would be the same as the version tox is using, but the tests still pass in a wide terminal, whereas they fail when run with tox.

praiskup commented 1 year ago

Tox looks OK now :-) at least in CI, the COLUMNS sensitivity is a known thing, sorry for that.

praiskup commented 1 year ago

I'd still prefer to have a test case, lemme know if you want me to create it.

Otherwise LGTM, this is an awesome contribution - thank you!

rrthomas commented 1 year ago

@praiskup Don't worry, I'll create the test case! I also have to test that the "match patterns", for compatibility with help2man, should be regexs, not plain text (this is what I've documented). I will also attend to your other comments, e.g. about pylint warnings.

rrthomas commented 1 year ago

Having some docs for the new --include feature in README.md would be awesome too.

I've done this now.

rrthomas commented 1 year ago

I have added a test; it is breaking Copr currently, I'm hoping just because I needed to add the new test file to MANIFEST.in, which I've now done.

rrthomas commented 1 year ago

Please can we have a release with this in? :)

praiskup commented 1 year ago

Done, cheers!

rrthomas commented 1 year ago

Many thanks!