Slurp errors to keep errcheck happy

rubenv / sql-migrate

SQL schema migration tool for Go.

MIT License

3.23k stars 280 forks source link

Slurp errors to keep errcheck happy #124

Closed quintenpalmer closed 5 years ago

quintenpalmer commented 5 years ago

Not sure if upstream is using errcheck, so likely will not publish this change to them, unless they are welcome to it. Publishing as its own PR to keep that bookkeeping separate.

There is nothing really to do with these errors.

There are no loggers in the top-level library code, and this is already in the rollback error control flow, so there is no erroring to signal (we already are).
The failure to print to stderr in the main sql-migrate doesn't seem to have any sane recourse.
We could consider a message in the failure to close the file descriptor, but this logic is going to be moved up to the library, which still doesn't have a logger, and this is already when we are going through the error path, so there is no error to return.

quintenpalmer commented 5 years ago

sorry, meant to push this against my fork, if you (upstream) are happy with this change, then would be nice to have this accepted to keep errcheck (https://github.com/kisielk/errcheck) happy.

quintenpalmer commented 5 years ago

If you're not interested in this change I can happily close it or handle the errors differently, definitely open to discussion.

rubenv commented 5 years ago

Hey,

I'm a fan of static analysis, so I definitely don't have a thing against errcheck. In fact, we should probably add it to the Travis tests.

All these assignments to _ do feel silly, but I guess that's the price we have to pay for it.

Would you mind hooking errcheck into the Travis config?

quintenpalmer commented 5 years ago

Awesome, glad to hear that you're up for satisfying errcheck. I'm not too familiar with Travis, I'll try to find some time soon to figure out how to add errcheck and run it as part of the Travis checks.

rubenv commented 5 years ago

Shouldn't be too hard, it's just a YAML-shaped shell script, so you'll probably just need to add a go get line and a line to run it.

quintenpalmer commented 5 years ago

sounds good, I'll give that a shot soon, thank you!

quintenpalmer commented 5 years ago

@rubenv sorry for the delay, let me know if these last two commits look good. There was one last error not being handled that I caught and I introduced the Travis config changes to run this. I verified that the errcheck check is running and is passing, you can see an example here: https://travis-ci.org/rubenv/sql-migrate/jobs/467150613

rubenv commented 5 years ago

Perfect, thanks!