(draft) use n0-error #3511
(draft) use n0-error #3511
Conversation
…\n\n- Add ./n0-error path dep to iroh-base and iroh-relay\n- Replace snafu/n0-snafu with n0-error in iroh-base (relay_url, key, ticket)\n- Begin iroh-relay port with node_info.rs (errors, ensure, constructors)\n- Add AGENTS.md contributor guide
…porting\n\n- Adjust path deps in iroh-base and iroh-relay\n- Re-introduce snafu temporarily for iroh-relay, begin migration in server.rs and node_info.rs\n- Port error types and call sites to n0-error in server.rs and node_info.rs\n- Keep workspace compiling (cargo check clean)
…[from]/#[std]/#[transparent] with #[error(from/std_err/transparent)] across migrated files\n- Keep RelayUrlParseError as struct; add proper field attributes\n- Ensure iroh-base and iroh-relay compile with updated n0-error API
…ere appropriate\n\n- Combine #[error(from)] + #[error(std_err)] into #[error(from, std_err)]\n- Use #[error(from_sources, std_sources)] for enums with mostly std/from sources (KeyParsingError, ParseError in iroh-base; DecodingError, SpawnError in iroh-relay)\n- Keep variant overrides (transparent, stack_err) where needed\n- Workspace compiles
| let input = input.as_bytes(); | ||
| if data_encoding::BASE32_NOPAD.decode_len(input.len())? != bytes.len() { | ||
| return Err(DecodeInvalidLengthSnafu.build()); | ||
| return Err(KeyParsingError::decode_invalid_length()); |
There was a problem hiding this comment.
it would be good if we can restrict these methods to be crate only, to not make them part of the public api
There was a problem hiding this comment.
thinking about this some more, we will want to be able to be able to configure visibility on a variant level, eg this error
enum MyError {
#[error(visibility(pub(crate))]
InternalOne,
// only visible in this module by default
InternalTwo,
// usable externally
#[error(visibility(pub))]
User { source: Box<dyn Error> },
}
| let input = input.as_bytes(); | ||
| if data_encoding::BASE32_NOPAD.decode_len(input.len())? != bytes.len() { | ||
| return Err(DecodeInvalidLengthSnafu.build()); | ||
| return Err(KeyParsingError::decode_invalid_length()); |
There was a problem hiding this comment.
I guess this means rust-analyzer "find-all-references" on KeyParsingError::DecodeInvalidLength is not going to find anything?
That's one of the main usability issues I have with snafu.
There was a problem hiding this comment.
Yes, this is true unfortunately. And I agree that fixing this would be a major improvement.
Unfortunately this has tradeoffs. We want to have a location (or backtrace or similar) field be included in all errors. So we have, I fear, only these options:
- Have the field be part of each enum variant (current situation). Then we can initialize..
- with a function (current PR) or a separate struct (eg the ...Snafu structs). Both are not find-all-references compatible.
- manually with e.g.
KeyParsingError:::DecodeInvalidLength { location: Default::default() }. This works with find-all-references but is more verbose at each error construction site.
- Wrap errors in a generic outer err. So e.g.
fn parse_key() -> Result<Key, Err<KeyParsingError>>. Then the location context can be tracked in the outer error and could be initialized likeKeyParingsError::DecodeInvalidLength.build(). However, IIRC this leads to trouble with the questionmark operator in some cases (can maybe be solved - I'll give it another try) and of course the signatures look more complex throughout the board.
I am undecided yet. Wdyt?
There was a problem hiding this comment.
First note that I'm spouting ideas without much knowledge of how things need to be wired up. So I might be suggesting nonsense.
Maybe 1.2 is workable. I think you can also write this as KeyParsingError::DecodeInvalidLength { ..Default::default() } which is nice if you have other fields to initialise on the enum.
Maybe another option could be to use yet another macro to create the structs itself, like Err!(KeyParsingError::DecodeInvalidLength).
It seems like 2 is much more the "Error + ErrorKind" type of approach. At first it does look appealing. But yeah, depends on the ergonomics of all the edge cases. Also matching on errors could become a little more awkward, I really like being able to write match statements without having to do the nested match err.kind() that std::io::Error makes you do.
There was a problem hiding this comment.
1.2 could also be MyError::Foo { field, loc: loc() } with use n0_error::loc if we aim for more brevity in the common case.
The generic error matching wouldn't be too bad: we could define it as struct Err<K>(pub K, Location) then you could match on err.0. I'll give it a try some time soon.
|
|
||
| let proxy_port = url_port(&proxy_url).context(ProxyInvalidTargetPortSnafu)?; | ||
| let proxy_port = | ||
| url_port(&proxy_url).ok_or_else(|| DialError::proxy_invalid_target_port())?; |
There was a problem hiding this comment.
why can't we use context here?
3 participants
Description
replace snafu and n0-snafu with n0-error, branch
Frando/rework-attrshad the LLMs give the conversion of iroh-base and iroh-relay a go, and they worked for a while.
With this we can have a look if we want this for real.
Breaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme