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

Log unhandled errors if they occur #508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikeschinkel
Copy link

@mikeschinkel mikeschinkel commented Aug 12, 2024

NOTE: This is a LARGE PR but it only does one thing. If you may plan to accept it, you should do before you add a lot of new code or it will just be too hard to resolve the conflicts. HOWEVER, this PR changes the API so to follow SemVer I think you would need to bump the major version number.


Purpose

This PR replaces finds almost all places where an error is returned by a function — but the Mutagen code just ignores the error — and it adds logging in case an error is returned.

I discovered these issues while I was making changes for my own needs on my fork for #505. My IDE kept complaining about unhandled errors, so I decided to fix them and move to a separate PR. As it turned out, fixing them was quite the rabbit hole, but I did so because I know having all unhandled errors logged will make for more robust software and potentially reveal edge-case bugs in the code you did not even realize where there.

Serendipitously DoltHub published a blog post entitled "What's the best Static Analysis tool for Golang?" that starts with an example of an unhandled error as justification for why handling all errors is so important. I say "serendipitously" because it was published after I wrote the code for this PR.

What the PR Changed

  1. The PR adds a package named must which includes functions for most of the places where errors were being ignored, such as Close(), Shudown(), etc. Then is replaces the code that looks like this:

    item.Close()
    

    With code that looks like this:

    must.Close(item,logger)
    
  2. In order to ensure a logger was always available, this PR adds:
    a. A logger property to every struct that has a method where unhandled errors occurred, or
    b. A logger parameter for every package function where unhandled errors occurred, and
    c. Ensured those properties were set and parameters were passed in all cases.

  3. In the very few cases that use interfaces I decided to handle the error manually so as not to modify the interface.

  4. In one case I could not find a reasonable way to get a logger so I just left a TODO comment.

Summary

I really hope you will consider accepting this. It was a few days of work to both find and fix all the case and to them get all the CI tests to pass. If you do, I think it will help you make Mutagen even more robust moving forward.

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.

1 participant