Show installed packages too

[jnwatts]

I wanted to see installed packages too. So I did. ;-)

[pbrisbin]

Hi. Thanks for this PR. I'm on the fence on including this extra output. On one hand, my idea of what wat is for is what did I just upgrade?. On the other hand, it could also be interpreted (as the README implies) as what did I do during/since that upgrade?.

I'm going to think a bit more on it. Sorry for the delay.

[jnwatts]

I would've like to use bash's getopt to more easily parse -i or something, but didn't want to break your scripts current assumption of a basic posix shell. For what I want, I can use an alias or wrapper script to always apply the environment variable. (Or I can just keep my own version of wat ;-)

[pbrisbin]

Thanks for taking a swing at this, and honoring my shebang choice.

I do think an option would be better, but it shouldn't need getopt, just a simple case "$1". I was also thinking of a more flexible --regex but the downside is you'd need to repeat and extend the default, vs just extending. So I think -i,--installed is the way to go.

Also, I would want to add a test for this before merging.

[jnwatts]

Understood, will head that route
On Jan 28, 2015 11:41 AM, "pat brisbin" notifications@github.com wrote:

Thanks for taking a swing at this, and honoring my shebang choice.

I do think an option would be better, but it shouldn't need getopt, just
a simple case "$1". I was also thinking of a more flexible --regex but
the downside is you'd need to repeat and extend the default, vs just
extending. So I think -i,--installed is the way to go.

Also, I would want to add a test for this before merging.

Reply to this email directly or view it on GitHub
#2 (comment).

[jnwatts]

Heh, cram is a neat little tool. :-) Implemented command line arguments and matching tests.

[pbrisbin]

Thanks very much for adding a comprehensive test (two in fact!) but I think it's a bit redundant and could therefore turn into a maintenance burden. I think it would be OK/better to just add a single additional case to the existing test that runs with -i and confirms the installed stuff appears. No need to re-create all of the existing cases with -i and --installed.

If you don't want to go through the textual-hassle just say so and I'd be happy to do it myself after merging this.

[jnwatts]

Alrighty: I wasn't sure how extensive or not it ought to be. I've very rarely ever dealt with automated code tests. I'll merge it back into wat.t

[pbrisbin]

Thanks for this. I commented on a few minor nits, but if you'd rather I merge this as-is then make those changes myself after, that's fine too. Just let me know.

[pbrisbin]

I've not seen this syntax before: any chance you could point me to it's name/specification?

What syntax are you referring to?

[jnwatts]

On Mon, Feb 2, 2015 at 3:32 PM, pat brisbin notifications@github.com
wrote:

I've not seen this syntax before: any chance you could point me to it's
name/specification?

What syntax are you referring to?

Oops, that was supposed to be a line-specific comment:

: ${PACMAN_LOG:=/var/log/pacman.log}
: ${SHOW_INSTALLED:=0}

What does the ':' mean?

[pbrisbin]

: is a command that ignores all its arguments and returns successfully. Without it, you would see the following:

${PACMAN_LOG:=/var/log/pacman.log}
# => error: /var/log/pacman.log command not found

It's a nice idiom and is shorter than

PACMAN_LOG="${PACMAN_LOG:-/var/log/pacman.log}"

which is how you'd do that without :.

[jnwatts]

Ah! Very neat :-) am going to enjoy using that one in the future.
On Feb 2, 2015 3:46 PM, "pat brisbin" notifications@github.com wrote:

: is a command that ignores all its arguments and returns successfully.
Without it, you would see the following:

${PACMAN_LOG:=/var/log/pacman.log}# => error: /var/log/pacman.log command not found

It's nice idiom and is shorter than

PACMAN_LOG="${PACMAN_LOG:-/var/log/pacman.log}"

which is how you'd do that without :.

Reply to this email directly or view it on GitHub
#2 (comment).

[pbrisbin]

This looks good enough to merge as-is. I'm OK with allowing SHOW_INSTALLED as an env var as well.

[jnwatts]

Cool, thanks :-)
On Feb 2, 2015 4:07 PM, "pat brisbin" notifications@github.com wrote:

This looks good enough to merge as-is. I'm OK with allowing SHOW_INSTALLED
as an env var as well.

Reply to this email directly or view it on GitHub
#2 (comment).