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

Expose testerror (and minor docs improvements) #14

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

ijackson
Copy link
Contributor

Closes #13

Apropos the discussion there, I think it's fine to expose that this is enum {}. We probably don't want to change that - in particular, we aren't likely to want to make this an alias for something else, nor are we likely to need to make it an opaque struct.

(Incidentally, I have a similar type in one of my own projects: https://salsa.debian.org/iwj/otter/-/blob/main/apitest/apitest.rs?ref_type=heads#L109. Mine is different to TestError because it displays error sources too, which is a different answer to the problem that Display is kind of broken for implementors of std::error::Error.)

@ijackson
Copy link
Contributor Author

Rebased to add my S-o-b.

Sorry, I hadn't noticed that this project used this convention (which I approve of!)

@wiktor-k
Copy link
Owner

There's still one issue with the clippy lint hopefully resolved with #15.

(Incidentally, I have a similar type in one of my own projects: https://salsa.debian.org/iwj/otter/-/blob/main/apitest/apitest.rs?ref_type=heads#L109. Mine is different to TestError because it displays error sources too, which is a different answer to the problem that Display is kind of broken for implementors of std::error::Error.)

Hah, that's cool too! Initially I went the route of Errors too but it didn't work for anyhow and I didn't like the anyhow dependency. I see you've taken another route and arrived with a different solution.

I'm wondering if it makes sense to somehow marry these two 🤔

(for anyhow another person suggested just using native anyhow features: #6 (comment)).

@ijackson
Copy link
Contributor Author

Rebased as suggested.

I'm wondering if it makes sense to somehow marry these two 🤔

Possibly. I guess one could make the anyhow dependency optional with a cargo feature. But that way lies features for all the non-directly-std-Error-implementing error crates.

I wrestled with AsRef a bit as well, IIRC, but evidently not successfully. And the thing in Otter isn't productised enough for use elsewhere right now. That's kind of why in another project I'm working on now, I thought I'd try out testresult :-).

@ijackson
Copy link
Contributor Author

Oh and:

(for anyhow another person suggested just using native anyhow features: #6 (comment)).

Having it panic on conversion can mean that when you look at it in a debugger, many of the local variables you want to inspect are still live - since the panic is called from the function that generated the error, rather than from main or somewhere after the stack has been unwound. I think it may also work better with panic=abort, which could be useful when testing stuff intended for embedded contexts.

So I think the uninhabited type is a good approach, which is useful in quite a few contexts, even thouth there's that backtrace feature in anyhow. (In Tor we also have a stacktrace-containing Bug type that we use for internal errors in production code, since we want to avoid panics.)

@wiktor-k wiktor-k enabled auto-merge July 16, 2024 15:38
@wiktor-k wiktor-k merged commit e18e2c2 into wiktor-k:main Jul 16, 2024
16 checks passed
@wiktor-k
Copy link
Owner

Thanks for providing me with more context. I'll be slowly digesting it.

As for testresult I plan to do a little bit of cleanups w.r.t. CI and docs and will release 0.4.1 shortly.

Thank you for your work and time. It's greatly appreciated! 🙇‍♂️

@ijackson
Copy link
Contributor Author

Thank you for your work and time. It's greatly appreciated! 🙇‍♂️

You're welcome, and thanks. It's been a pleasure :-).

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.

Let's expose TestError
2 participants