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

Add note about using stdout #44

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Rushmore75
Copy link

Add Note: use - as [file] for stdout, letting the user know they can pipe to stdout without an extra flag, instead of writing to a file.

Copy link
Member

@9ary 9ary left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution!

In addition to my inline comment, the commit message is no good. Please take a look at the existing history for examples of good commit messages.
Here's one that you could use verbatim:

Document magic output filename behavior

The special output filename `-` will be interpreted to mean stdout.
Document this behavior in the help text.

src/main.rs Outdated
@@ -23,7 +23,7 @@ mod xwrap;
use crate::xwrap::Display;

fn usage(progname: &str, opts: getopts::Options) {
let brief = format!("Usage: {progname} [options] [file]");
let brief = format!("Usage: {progname} [options] [file]\nNote: use - as [file] for stdout");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right place to document this, the help text should probably be reworked a bit to accommodate this. Ideally, the [file] positonal argument should have its own entry above the options section describing its behavior (both normal and special).
Also, since you're changing the help text, you have to update the readme to match.

Added manpage using groff markdown using the man macro.
Using `groff -Tascii -man shotgun.man` to transpile.
This page can be viewed using the `man` command, thus giving you the
program's manual.
@Rushmore75
Copy link
Author

I put up some more commits, they add a manpage. I'm not sure how you would like to install the man page, I wrote some code which I'll put in another PR doing it in the build.rs file, but I'm not super happy with it. cargo install (installing under ~/.cargo/) doesn't really have a method for man pages. Perhaps it's something that's done when its published to AUR?

@Rushmore75 Rushmore75 mentioned this pull request Sep 1, 2023
Copy link
Member

@Lonami Lonami left a comment

Choose a reason for hiding this comment

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

We think the manpage is too much for a tool as self-contained as shotgun, and adds other questions we're not ready to resolve yet.

@9ary's idea was to document it above the Options. So if we take the current --help form the README:

Usage: shotgun [options] [file]

Options:
    -i, --id ID         Window to capture
    -g, --geometry WxH+X+Y
                        Area to capture
    -f, --format png/pam
                        Output format
    -s, --screen        Capture the current screen
    -h, --help          Print help and exit
    -v, --version       Print version and exit

The idea, as I understand, is something closer to (with exact wording TBD):

Usage: shotgun [options] [file]

    file                Output file, or - for stdout

Options:
    -i, --id ID         Window to capture
    -g, --geometry WxH+X+Y
                        Area to capture
    -f, --format png/pam
                        Output format
    -s, --screen        Capture the current screen
    -h, --help          Print help and exit
    -v, --version       Print version and exit

I'm not familiar with getopts, but I'd imagine it's possible to document free matches somehow.

You'll also need to update the README once we settle on the correct message.

@9ary
Copy link
Member

9ary commented Sep 1, 2023

In general I'm not opposed to adding a man page but I think it should be generated from the source code somehow.
Maybe this would require switching to a different argument parsing library.

The reason why shotgun doesn't have a manpage is that it's so simple that I didn't think it was worth the hassle of figuring out the tooling.

At any rate, if we're adding a man page then the related stuff from #45 should be in the same PR. So let's either keep this PR focused on improving the existing documentation and move all the manpage stuff to #45, or merge #45 into this one.

@Rushmore75
Copy link
Author

Ok, in contrast to what Lonami I feel that every program should have a manpage, even if the --help text is short, as it's easier to search a manpage because it opens in less (or similar program).

It would be super cool if there is a parsing library that automatically generates the manpage! IDK how required that is, as I'm not sure as to how often you add options to the program. As for editing the manpage it's not too hard, even if it's in weird groff markup, for example: my nvim instance automatically had syntax highlighting available for it.

If the idea of a manpage does get tossed, I think what Lonami has in that comment would work well for the help text.

Bandwhich is another rust program, which has a manpage, https://github.com/imsnif/bandwhich/blob/main/Makefile#L18 they add it in a makefile tho, so not that nice of a solution :/

@expectocode
Copy link
Member

My 2c: shotgun doesn't need a manpage. If we are going to add one, let's keep the maintenance burden and complexity to an absolute minimum.

@Lonami
Copy link
Member

Lonami commented Sep 1, 2023

it's easier to search a manpage because it opens in less

You can pipe the --help to | less, too!

If the idea of a manpage does get tossed

I'm not against it (I'm barely involved in shotgun myself). But this PR's focus seemed to be to add a note about stdout, and the --help is the easiest place to do it in. A manpage could still be added in e.g. #45 if we find it worthwhile and not too cumbersome to maintain.

I just think manpages should stay out of this PR (and if we prefer to have the note on using - for stdout only in the manpage, then this PR could be closed).

@bb010g
Copy link
Contributor

bb010g commented Sep 1, 2023

I'm fine with a manual, but I would prefer it to be submitted in a separate patchset.

@Rushmore75
Copy link
Author

ok, then it looks like all the "man" stuff can stay in #45, and here can just be a note, if any, in the --help text :)

Also removed manpage from this PR
@Rushmore75
Copy link
Author

Added a commit to make this PR just about helptext

Copy link
Member

@Lonami Lonami left a comment

Choose a reason for hiding this comment

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

Please rebase to clean-up the commit history. It's gotten quite messy and is not up to our standards.

Comment on lines +19 to +20
Options:
-i, --id ID Window to capture
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong.

Comment on lines +26 to 28
let brief = format!("Usage: {progname} [options] [file]
\n file: use - as [file] for stdout");
let usage = opts.usage(&brief);
Copy link
Member

Choose a reason for hiding this comment

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

Does opts not have a way to document free-standing options? Abusing the brief for that is weird as getopts likely won't be able to do any layout on it.

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

5 participants