-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix: add track_caller attr on panicking methods #96
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! I'm a little torn on this one. On the one hand, I definitely agree it makes the stack traces easier to read for the users of the library. But on the other hand, it makes the stack traces less useful when trying to track down bugs in Ropey itself (e.g. if someone runs into a panic they can't repro, but they do have a stack trace they can include in the report). I'll have to think on this one for a bit. But regardless, I appreciate the time you took to put together and submit the PR! |
I'm playing around with First of all, if you enable stack traces, the whole stack trace gets printed regardless. In retrospect I should have realized this, since it always goes deep into stdlib on panics regardless. In other words, my concern about stack traces is completely unwarranted: you always get the full stack trace. So nevermind. But second, it reports that the error (in my linked example) is due to unwrapping This makes sense from a technical perspective: it's just printing the error message specified in the actual panic. But it does mean just adding the In short: I think we should do this, but it will involve additional work beyond this PR. I'll have a bit more of a think about it, and I'll also need to give the code a closer review. But I think what probably makes sense is to merge this PR as-is, and then we can start working on improving the panic messages to make sense to users of the library. |
I totally agree that I've also taken the liberty to remove custom panic messages that include lengths. I can revert anyway. To me, these take a lot of space and don't provide much error value. You can get this info but printing it yourself when debugging. |
When using panicking versions of the
Rope
methods, IMO it's better to get the location where you didn't respect the invariants that lead to the panic over the location of the methods inropey
's code.