-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
manuals: Remove trailing spaces #1473
base: main
Are you sure you want to change the base?
Conversation
7549ae0
to
bbdcfa1
Compare
This does not change the rendered ascii at all. Signed-off-by: Graham Percival <[email protected]> Sponsored by: Tarsnap Backup Inc. Pull Request: freebsd#1473
Low priority (although very simple) PR. This does not change the rendered ascii at all. @mhorne |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wonderful, inappropriate whitespace is just unbelievably offensive.
@@ -56,7 +56,7 @@ WMI status device. | |||
.El | |||
.Sh SYSCTLS | |||
The following sysctl node is currently implemented: | |||
.Bl -tag | |||
.Bl -tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Bl -tag | |
.Bl -tag -width "dev.acpi_wmi.%d.bmof" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I promise it's within the scope :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... agree to disagree? The subject line is "remove trailing spaces", and anybody checking it with git show --word-diff-regex=.
(or on github) can easily verify that's the case.
Adding a -width
would qualify as "minor cleanup" (as per the following comment), but I wouldn't say that it's part of "remove trailing spaces".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can absolutely agree to disagree! My suggestions are just suggestions, and it is an honor to be here.
My concern is that if you don't, you're proposing making a commit to a broken line which still leaves it broken.
With my current understanding, I can't approve of committing broken code to the tree, but I am always desiring new information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that .Bl -tag
without -width
is a warning:
mandoc: share/man/man4/acpi_wmi.4:59:2: WARNING: missing -width in -tag list, using 6n: Bl -tag
However, that's not a warning that this commit adds to the project. The warning is already there; this commit improves that file (slightly) by removing the trailing whitespace. Nothing more, nothing less.
Yes, it doesn't fix the warning. But with over 2000 mandoc warnings in the git tree, there's likely going to be warnings in files that we're editing. I mean, if the warning happened 5 lines earlier or later, we wouldn't notice at all.
...
I suppose that a bigger thing is how to approach PRs in a large project. In my experience, large projects are limited by the number of reviewers.
As it is, this PR / commit is extremely easy to understand. The subject line says "remove trailing spaces". Let's check the diff: is that accurate? Yes; it's easy to verify. Do we want to remove trailing spaces? Yes (I assume); they provide no benefit in man or mdoc.
My goal is for the reviewer -- and anybody looking at the git history after merging -- to understand it with the minimum amount of effort.
If a freebsd developer says they don't like removing the whitespace from the already-broken .Bd -tag
(complete with trailing space), then I think the solution would be to leave that file unchanged; not to add syntax changes to a whitespace-only commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: #1477
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider applying your skills to the logic preventing incompatible options in man.sh, everyone's been stumped for a while on that one
I assume that you're talking about usr.bin/man/man.sh
? And the "Check the args for incompatible options" section on line 615?
It looks fine to me at first glance. What's the problem?
(I couldn't find any bugzilla issue about this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If you actually test it, it doesn't work, and we have had several iterations of people fiddling with it and giving up. Try man -Kk whatever
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. That's a consequence of using getopts
.
man -Kk whatever
means -K
with the option k
, and an extra whatever
that's ignored (I think).
Even if you split it up into man -K -k whatever
, that's still interpreted as:
- option
-K
with the argument-k
(so the regex is "-k
") - and there's an extra
whatever
on the command-line, which I think is ignored.
If you give it a getopt-compatible command line, the "incompatible options" are fine:
$ ./usr.bin/man/man.sh -K whatever -k
Incompatible options: -K and -k
Usage:
man [-adho] [-t | -w] [-K regexp] [-M manpath] [-P pager] [-S mansect]
[-m arch[:machine]] [-p [eprtv]] [mansect] page [...]
man -f page [...] -- Emulates whatis(1)
man -k page [...] -- Emulates apropos(1)
$ ./usr.bin/man/man.sh -k -K whatever
Incompatible options: -K and -k
Usage:
man [-adho] [-t | -w] [-K regexp] [-M manpath] [-P pager] [-S mansect]
[-m arch[:machine]] [-p [eprtv]] [mansect] page [...]
man -f page [...] -- Emulates whatis(1)
man -k page [...] -- Emulates apropos(1)
As for how to fix it... we could try avoid the man -K -k whatever
case by complaining if REGEX begins with a -
. But that wouldn't fail if somebody writes man -Kk whatever
.
...
I'm tempted to bail if there's any unused args on the command-line -- I've seen that used successfully in some other utilities. But I'm not (yet) sufficiently familiar with man.sh
to know if that would cause a problem with any valid workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that a bigger thing is how to approach PRs in a large project. In my experience, large projects are limited by the number of reviewers.
As it is, this PR / commit is extremely easy to understand. The subject line says "remove trailing spaces". Let's check the diff: is that accurate? Yes; it's easy to verify. Do we want to remove trailing spaces? Yes (I assume); they provide no benefit in man or mdoc.
My goal is for the reviewer -- and anybody looking at the git history after merging -- to understand it with the minimum amount of effort.
If a freebsd developer says they don't like removing the whitespace from the already-broken
.Bd -tag
(complete with trailing space), then I think the solution would be to leave that file unchanged; not to add syntax changes to a whitespace-only commit.
@gperciva I appreciate your thoughtful approach to scoping PRs. Indeed it does make reviewing easier.
In a case like this, where review reveals a secondary issue slightly outside of the initial scope, my preference is for you to make the fix in a second commit, but push it to the same branch/PR. This way I can still land the single PR with the same amount of work, and we do not lose any improvements.
Put differently, minor issues/cleanup discovered during the review of a PR can be considered within the scope of the PR, implicitly. But of course such changes should be made in a separate commit where they are functionally different, for the benefit of future readers of the log. It is common with source code changes for reviewers to request style/code clarity improvements "while here", which are then committed separately to the functional change.
For larger issues uncovered during review (such as the question of list alignment in vmem.9) it is fine to say "WONTFIX" or "will fix in a separate PR". Of course, what "larger" means is a sliding scale, requiring judgement in each case...
So, please create a second commit adding the missing -width
arguments, and I will take it as part of this PR :)
Thanks for your work!
@@ -56,7 +56,7 @@ WMI status device. | |||
.El | |||
.Sh SYSCTLS | |||
The following sysctl node is currently implemented: | |||
.Bl -tag | |||
.Bl -tag | |||
.It Va dev.acpi_wmi.%d.bmof | |||
Managed Object Format (MOF) blob. | |||
You can obtain human readable output by bmf2mof in bmfdec tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way, while we're here, We can get that url converted to a Pq Lk? That makes it actually followable in most software. It's still within the scope of "minor cleanup (markup)", and I'm not (a SME or) aware of anything else that really needs done to this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! (although I still don't think it's in scope of this particular PR)
I think that's a good candidate for a script, though.
$ grep "http://" -- $(cat man-pages.txt) | grep -v "\.\\\\\"" | grep -v "Lk " | wc -l
169
$ grep "https://" -- $(cat man-pages.txt) | grep -v "\.\\\\\"" | grep -v "Lk " | wc -l
922
Amusingly, there's quite a notable amount where people used a .Pa
macro in front of it -- 55 for http:
, and 24 for https:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing the doc like I'm supposed to do while we're proposing changing it, it turns out that repo is packaged, so I've moved it out of the sysctl and into the example where it's used and linked it here #1477.
Just a quick note that I've seen your comments, but I haven't gotten around to this yet because I've been working on the registration system for BSDCan 2025. I'll have more time after Sat. |
No description provided.