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

[WIP] Replace panic! and unreachable! with internal_error! #2409

Closed
wants to merge 6 commits into from

Conversation

matssigge
Copy link
Contributor

I've gone through the "low risk" packages listed in #2046 and replaced panic! and unreachable! with internal_error! instead. For panic!, I've just replaced the name and haven't changed any parameters. For unreachable, there are two different cases:

  1. A call with no parameters, i.e. unreachable!(). In this case I replaced it with internal_error!("unreachable"), so that it's still clear that this should never happen.
  2. A call with an error string, e.g. unreachable!("Any other pattern should have given a parse error"). In this case, I've added "unreachable: " at the beginning of the string, again to make it clear that the code isn't expected to ever run.

I just realized that I haven't used user_error! at all, so I will go through previous uses of panic! and try to determine where user_error! is better than internal_error!.

@matssigge
Copy link
Contributor Author

I had several attempts at this which all failed because I didn't manage to run the test suite locally. Since I've realized I don't have time to spend on this right now, I'm closing the PR again.

@matssigge matssigge closed this Feb 10, 2022
@JanCVanB
Copy link
Collaborator

@matssigge Thank you for attempting this! Would you like any help running the test suite locally? Our GitHub-hosted CI tests have also gotten better this year, if that's sufficient.

@matssigge
Copy link
Contributor Author

@JanCVanB I'm sorry, but I don't have the time to work on this right now. I was between projects at the beginning of the year, giving me some free time, but since February I'm working full time, and now I don't have the bandwidth.

@JanCVanB
Copy link
Collaborator

@matssigge That's okay, congratulations on your full-time work! I hope that your and Roc's paths cross again someday 🙂

@JanCVanB JanCVanB deleted the use_error_macros branch September 28, 2022 11:55
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.

2 participants