-
Notifications
You must be signed in to change notification settings - Fork 240
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
A couple more manpage improvements #251
Conversation
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 looks like a nice cleanup. My only concern is that it would be nice to have the exit value for the system calls if those fail, which we lose with the move away from autodie
for them.
dev-bin/make-man-pages.pl
Outdated
@@ -2,19 +2,18 @@ | |||
|
|||
use strict; | |||
use warnings; | |||
use autodie qw( :all ); | |||
use autodie; |
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.
If we remove the :all
, it would probably be worth including $? >> 8
in the system
error message as the exit code may be useful in diagnosing a failure.
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'm happy to do that, but also happy to keep the :all
as well (it has a dependency IPC::System::Simple
, but that's not a big problem). I was just trying to converge with geoipupdate a bit, which doesn't have :all
and has those or die
in system. So, no strong feelings myself, and happy to go with whatever you think is best!
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.
Yeah, I am not sure why these two scripts diverged on autodie
. Using :all
seems the simplest to me. The geoipupdate
script appears to check every binmode
, print
, close
, etc., which seems excessive when we can just use autodie
.
Rename arguments and expand system() calls into multiple lines, to match what geoipupdate's make-man-pages.pl does. This will become handy in the next commit, as we add more arguments to pandoc/lowdown, as well as in future convergence attempts with geoipupdate. While at it, also pass the code through perltidy, which changes whitespace in a couple of other places.
Both pandoc and lowdown now support passing document metadata in command-line arguments with -M, so do that instead of writing a temp file on disk. Even pandoc 1.12.2.1 from e.g. Ubuntu 14.04 seems to be supporting this, so it should be safe enough.
With the latest changes in lowdown (0.8.1), the outputs have only minimal differences. lowdown is more portable (small C, rather than Haskell) and thus easier to install into systems. Ef pandoc is still there, just second in preference. This only matters on systems that have both lowdown and pandoc installed, so overall not a hugely impacting change. Effectively this only matters for e.g. Linux distributions etc.
cbef50b
to
e631bd9
Compare
Thanks! |
Follow-up to #248. Sorry to do this peacemeal, but lowdown started supporting
-M
only with 0.8.1, released… today. (I wouldn't expect anyone generating manpages to not have a latest lowdown. I don't even know if there are others besides me ;)). On the plus side, the changelog entry for 1.5.1 is already there and doesn't need an update? :)I hope to converge geoipupdate's and libmaxminddb's
dev-bin/make-man-pages.pl
eventually, but while one of the commits below is in that direction, I'm not there yet. If you have more copies of that in other MaxMind projects, happy to incorporate those as well and have one source for all.Thanks for the consideration!