[style] Use error instance rather than an error string

[coolaj86]

Recently we had a PR that was necessary, but had some code style issues that we'd like to fix:

The PR in question: https://github.com/mholt/archiver/pull/231/files

What would you like to have changed?

There are a few ways I think could work really well:

1. Follow the style of os.IsNotFound
   • Let's add function something like IsIllegalPath in archiver.go
   • Let's replace lines like strings.Contains(err.Error(), "illegal file path") with IsIllegalPath
2. Follow the style of csv.ParseError
   • Let's add a struct IllegalPathError to archiver.go
   • Let's replace things like fmt.Errorf("illegal file path: %s", filename) with the use of that error
3. Let's do both!

Why is this feature a useful, necessary, and/or important addition to this project?

We just want to make the code more durable and maintainable.

[ibraimgm]

I can take a look at this one, if you wish.

[coolaj86]

Thanks @ibraimgm, that would be great.

[ibraimgm]

Done. Please take a look at #246 and let me know if you any fixes are needed. BTW, please consider adding the "hacktoberfest" topic to this repo, if you can.

[mholt]

If you want to export error types/values, you might consider using %w to wrap errors. (I don't have a strong preference either way, but that's usually how Go does it; most of the std lib was written before error wrapping was standard so you won't see it there too much.)

[ibraimgm]

@mholt Not sure how wrapping itself will help in this case (maybe I'm missing something?).

I initially thougth about also adding a generic error value InvalidPathErr (with a empty Filename) and change the Error method accordingly. This would (at least in theory) allow the user to use errors.Is.Not really sure if it is worth the trouble, since we already provide IsInvalidPathError...

[coolaj86]

@ibraimgm Did you see my comment at https://github.com/mholt/archiver/pull/246#issuecomment-703280843 about https://github.com/mholt/archiver/blob/master/error.go ?

[ibraimgm]

@coolaj86 Sorry by the delay, I was out these last day and didn't have easy access to my messages. Did you create a new issue to track the checking by type instead of by the string? If so, please assign it to me.

BTW, please consider to opt-in to Hacktoberfest by following these instructions. They changed the rules this year, it is a bit more troublesome now.

[coolaj86]

I added hacktoberfest-accepted to the commit. Since this is a personal repo I don't have access to add it as a topic to the repo.

It's good that they changed the way it works. There were tons of spam commits going through - like this one: https://github.com/webinstall/webi-installers/pull/138

[ibraimgm]

Ok, using an error instance is not as easy as it seems at first glance. The problem is that the error is sometimes wrapped (for example, zip.go, line 257) and since it is wrapped, we "lose" the original error type and, as expected, IsIllegalPathErroris not able to recognize the error.

I think we can solve this by using the error wrapping functionality from go 1.13 (basically, combining %w and errors.Is), but currently minimum version of the package is go 1.12... Is it ok to me to bump this and implement the improved error checking? Or we have some special case/scenario where go 1.12 must be used?

It's good that they changed the way it works. There were tons of spam commits going through - like this one: webinstall/webi-installers#138

Now I understand the rule change. Unbelievable.

[coolaj86]

I think it would be okay to bump the minimum version, but it also sounds that perhaps we're fine to leave well-enough alone.

I'm going to close this out, noting that I'm still open for the alternate solution but I don't consider it a priority.