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

Restore man page generation (alternative to #202) #206

Closed
wants to merge 2 commits into from
Closed

Conversation

ssbarnea
Copy link
Member

Use argparse-manpage to generate the manpage.

@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label Jan 15, 2023
@ssbarnea ssbarnea force-pushed the fix/mkdocs branch 2 times, most recently from c396086 to 7e8df7a Compare January 15, 2023 15:18
@ssbarnea ssbarnea enabled auto-merge (squash) January 15, 2023 16:58
Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@ssbarnea

  • This produces are partly broken man page. I'm attaching an annotated screenshot of the local build result (from tox -e pkg) with details, below.
  • The new man page is not on par with the old one content-wise, no (or broken) usage examples, no links, no authors/attribution, no synopsis advertising --inline and --partial as most important arguments to know about. All of things that made the existing man page human are out the window now. This new page only serves the "has to have a man page" rule of e.g. Debian, nothing more.
  • Asciidoc is one of the best options around to produce man pages from markup, it is maintained, the syntax is close to Markdown, it was and is today a fine choice to maintain a man page targetting humans.
  • Output location man/ansi2html.1 is not ideal for when the repository does not contain a folder man after this, e.g. because there is no file man/.keepdir. It seems to work, but why put the file in an otherwise empty non-existing folder.
  • The addition of allowlist_externals = sh seems either unrelated or a leftover from experiments. My vote for removal or extracting a dedicated pull request.
  • You're closing my pull request Restore man page rendering #202 without a true chance for a discussion. I repeat, this is not how I want to collaborate.

ansi2html_man_page_Screenshot_20230115_211533

@ssbarnea
Copy link
Member Author

@ssbarnea

  • This produces are partly broken man page. I'm attaching an annotated screenshot of the local build result (from tox -e pkg) with details, below.
  • The new man page is not on par with the old one content-wise, no (or broken) usage examples, no links, no authors/attribution, no synopsis advertising --inline and --partial as most important arguments to know about. All of things that made the existing man page human are out the window now. This new page only serves the "has to have a man page" rule of e.g. Debian, nothing more.
  • Asciidoc is one of the best options around to produce man pages from markup, it is maintained, the syntax is close to Markdown, it was and is today a fine choice to maintain a man page targeting humans.
  • Output location man/ansi2html.1 is not ideal for when the repository does not contain a folder man after this, e.g. because there is no file man/.keepdir. It seems to work, but why put the file in an otherwise empty non-existing folder.
  • The addition of allowlist_externals = sh seems either unrelated or a leftover from experiments. My vote for removal or extracting a dedicated pull request.
  • You're closing my pull request Restore man page rendering #202 without a true chance for a discussion. I repeat, this is not how I want to collaborate.

ansi2html_man_page_Screenshot_20230115_211533

I will work on fixing these tomorrow. BTW, how can I display the man page locally without installing it?

For what platform are you building a system package? I would like to setup a pipeline to do so. I did something like this for rpm building using packit on few projects.

@hartwork
Copy link
Collaborator

BTW, how can I display the man page locally without installing it?

@ssbarnea that would be man ./man/ansi2html.1 or man man/ansi2html.1 — I think the slash makes man understand that it's dealing with a file and not a page name.

For what platform are you building a system package? I would like to setup a pipeline to do so. I did something like this for rpm building using packit on few projects.

@ssbarnea could you elaborate, I'm not sure I understand the question. I maintain the packaging of ansi2html in Gentoo Linux at https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-python/ansi2html/ansi2html-1.8.0-r1.ebuild . I would expect that you want zero Gentoo in your pipeline.

@ssbarnea
Copy link
Member Author

@hartwork I can generate the man page at any location but if I do not restore exactly the same location, any consumer will need to update his build scripts. This makes me think that you want me to restore the empty folder with .gitignore on it and keep the location. Am I right?

Re other issues, I am working on them, already fixed several.

@hartwork
Copy link
Collaborator

@hartwork I can generate the man page at any location but if I do not restore exactly the same location, any consumer will need to update his build scripts.

@ssbarnea the change would be trivial, so I would not mind personally.

This makes me think that you want me to restore the empty folder with .gitignore on it and keep the location. Am I right?

I was not intending to push for man/.gitignore in particular, but it's maybe the best solution to that very problem.

@hartwork hartwork changed the title Restore manpage generation Restore manpage generation (alternative to #202) May 1, 2023
@hartwork hartwork changed the title Restore manpage generation (alternative to #202) Restore man page generation (alternative to #202) May 1, 2023
@hartwork
Copy link
Collaborator

Here's an updated screenshort of what I see in the man page rendered by latest commit 248c8d6 in here:

Screenshot_20230526_162705

And this is the previous output as #202 would restore:

Screenshot_20230526_163226_old

I guess we're at a point now where the difference is marginal and both versions have pros and cons. I'll disable auto-merge now, then approve the pull request, and leave it to you @ssbarnea to push the merge button. It would allow us to get back to a releasable state.

@hartwork hartwork disabled auto-merge May 26, 2023 14:39
Use argparse-manpage to generate the manpage.
@hartwork
Copy link
Collaborator

Rebasing onto latest main using GitHub's button for it…

@hartwork
Copy link
Collaborator

@ssbarnea any thoughts on #206 (comment) above?

1 similar comment
@hartwork
Copy link
Collaborator

@ssbarnea any thoughts on #206 (comment) above?

@ssbarnea
Copy link
Member Author

@hartwork I do not have bandwidth to work on this, someone else will need to to the work, sorry.

@ssbarnea ssbarnea closed this Sep 12, 2023
@hartwork
Copy link
Collaborator

hartwork commented Sep 12, 2023

@ssbarnea while we have an "At least 1 approving review is required to merge this pull request." policy around here plus only regular Collaborator permissions for me, without you I literally cannot do anything: not merge pull requests, not push to master directly, not release to PyPI. Can you do something about that at least? Right now I have both hands tied behind my back.

@ssbarnea ssbarnea deleted the fix/mkdocs branch December 11, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants