Return proper error when given file/directory not found

[prettybits]

This fixes #220, which has been introduced with the recent changes to support glob arguments internally.

An explicit check for no matches was missing, which I added to return a fatal error. The message reads in a way that suggests that matching was involved instead of just saying "not found", is that OK?

If I'm not missing anything the possibility to supply glob patterns when the shell doesn't already expand them isn't yet mentioned in the README or documentation, should this be added?

[richardlehane]

RE:

If I'm not missing anything the possibility to supply glob patterns when the shell doesn't already expand them isn't yet mentioned in the README or documentation, should this be added?

Do you have examples? Is it just a README change?

This is a good point. I mentioned globs in the Changelog but neglected to update any other documentation.

Some docs that need updating:

• usage section of README
• wiki (especially Identifying file formats)
• usage section of itforarchivists.com/siegfried

[prettybits]

The variable name changing is a bit excessive for relatively new/reviewed code. Without a compelling rationale for matches, files is also a popular choice e.g. files, _ := filepath.Glob(v). But personal opinion/feelings aside, it also adds a layer of complexity to the diff for review as its only a one-line/three-line change.

I was hesitating before changing the variable names and I can understand if this feels a bit invasive maybe? Especially since they have just been recently introduced as you say. Would you prefer me to leave variable names as is in the future and possibly open them up for discussion in the PR itself?

I also wouldn't have changed them if it would have involved more lines of code, I thought 10 or so lines total would still be little enough complexity to see the core change, sorry. But to give context to the motivation, the function signature itself chooses that name (func Glob(pattern string) (matches []string, err error)) and globs to me signifies a collection of glob patterns instead of resulting matches - checking that explicitly against nil felt wrong to me in the moment. files would also be OK I guess, even though directories could also match. Do you want me to revert that part of the change for now?

Do you have examples? Is it just a README change? or are there other instances that we might want to write code for? As ever the devil seems to have been in the detail for what I thought was a simple change. (I should have reconsidered how to write unit tests for this one at the time!)

Richard already listed everything I was thinking of, no code changes intended. :)

[ross-spencer]

I was hesitating before changing the variable names and I can understand if this feels a bit invasive maybe? Especially since they have just been recently introduced as you say. Would you prefer me to leave variable names as is in the future and possibly open them up for discussion in the PR itself?

I think they're fine. And with the additional explanation, I can understand why the change, it looks less like opinion -- and your perspective that it's a good rule of thumb to follow the function signature for naming conventions is a good one, I can adapt this into Golang code-review guidelines. I've been dealing with reviews/PRs long enough that even though it doesn't feel great, I can handle it. It was something for me to raise to think about in future projects we work on with less experienced folk. It's a slippery slope to go back and forth over these things but your suggestion is good - to document them first time around in the PR notes is an empathetic way to do this. To separate things like this out into two commits is always a good idea too:

1. The fix.
2. Minor refactor/variable renaming, etc.

[richardlehane]

closing this as resolved in https://github.com/richardlehane/siegfried/pull/229