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

Format man page #102

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Format man page #102

merged 4 commits into from
Sep 2, 2022

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Aug 8, 2022

This PR improves the format of the man page.

@Honny1 Honny1 marked this pull request as ready for review August 9, 2022 13:36
@Honny1 Honny1 force-pushed the format-man-page branch 2 times, most recently from 8489242 to 2811aae Compare August 19, 2022 11:46
@Honny1
Copy link
Member Author

Honny1 commented Aug 22, 2022

/packit build

Copy link
Member

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

It's a great improvement.

But, the generated man page has some problems with formatting of command line options. For example it shows d, --debug instead of -d, --debug. Also, the FILE positional argument isn't visible.

"LOG LEVELS:\n"
"\tDEBUG - Detailed information, typically of interest only for diagnosing problems.\n"
"\tINFO - A confirmation that things are working as expected.\n"
"\nLOG LEVELS:\n"
Copy link
Member

Choose a reason for hiding this comment

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

For these types of strings where you want to preserve whitespace etc. a triple-quoted string """ ... """ could work better.

Comment on lines 24 to 25
"\tWARING - An indication that something unexpected happened, or a signal of"
" a possible problem in the future. The software is still working as expected.\n"
" a possible problem in the future. The software is still working as expected.\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

WARNING

@@ -85,7 +101,7 @@ def prepare_parser():
default="WARNING",
choices=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"],
help=(
"write debug information to the log up to the LOG_LEVEL."
"write debug information to the log up to the LOG_LEVEL. (default: %(default)s)"
Copy link
Member

Choose a reason for hiding this comment

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

This default section causes that there is the default described twice in the man page.

else:
default = action.dest.upper()
args_string = self._format_args(action, default)
parts.append(f'{", ".join(action.option_strings)} {args_string}')
Copy link
Member

Choose a reason for hiding this comment

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

There is too much braces and quotes on this line, which makes it harder to read. I think it would be more readable if the ", ".join(action.option_strings) part would be extracted to a variable.

@pep8speaks
Copy link

pep8speaks commented Aug 29, 2022

Hello @Honny1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 26:100: E501 line too long (156 > 99 characters)
Line 28:100: E501 line too long (118 > 99 characters)
Line 30:100: E501 line too long (103 > 99 characters)
Line 41:100: E501 line too long (133 > 99 characters)

Comment last updated at 2022-08-31 12:57:44 UTC

@jan-cerny
Copy link
Member

@Honny1 What has changed? Have you addressed the feedback?

@Honny1
Copy link
Member Author

Honny1 commented Aug 30, 2022

@jan-cerny Yes, I have addressed the feedback. I am currently trying to replicate the problem you described in the first place. But I can't reproduce the problem.

@Honny1
Copy link
Member Author

Honny1 commented Aug 30, 2022

@jan-cerny I have successfully reproduced the problem with the formatting of command line options. It looks like this is caused by some bug in the old version of the python3-sphinx (4.1.2) package. I'm using python3-sphinx (4.4.0) and it looks fine. What we can do with this problem?

@Honny1 Honny1 requested a review from jan-cerny August 30, 2022 15:13
@Honny1
Copy link
Member Author

Honny1 commented Aug 31, 2022

@jan-cerny I found the main cause of the problem with the formatting of command line options. In generated man page are missed white spaces before the parameter texts. That's why man does not show the first dash or FILE text.

Here is part of the diff in the man pages generated by the different versions of the packages:

95c97
< .B \-d, \-\-debug
---
> .B\-d, \-\-debug

I am currently working on a fix.

@Honny1
Copy link
Member Author

Honny1 commented Aug 31, 2022

@jan-cerny The fix is uploaded. You can test it.

@jan-cerny
Copy link
Member

Yes, it could be caused by the version. I'm using python3-sphinx-4.1.2-3.fc35.noarch. But, your fix didn't help, I still have the issue with missing dashes in options in the generated man page. It still generates lines like .B\-d, \-\-debug.

@Honny1
Copy link
Member Author

Honny1 commented Sep 1, 2022

@jan-cerny How did you generate a man page? The default behaviour of sphinx-build is to only read and parse source files that are new or have changed since the last run. My fix changed the way how arguments are parsed.
So if you just run sphinx-build -b man . TARGET_DIR. Nothing should be changed on the generated man page. I suggest you regenerate the man page with this command sphinx-build -b man -E . TARGET_DIR.

@jan-cerny
Copy link
Member

@Honny1 Great! That was it! I was simply expecting that it overwrites existing files. If I do some cleaning and remove them before, it works fine.

@jan-cerny jan-cerny merged commit e4b4a1c into OpenSCAP:master Sep 2, 2022
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

3 participants