ada-url / rust

Rust bindings for Ada URL parser
https://crates.io/crates/ada-url
Apache License 2.0
86 stars 12 forks source link

Improve error handling ergonomics (and docs around it) #62

Closed alexpovel closed 8 months ago

alexpovel commented 8 months ago
alexpovel commented 8 months ago

I noticed the example in the README not compiling, for two and a half reasons (2 errors, 1 warning). I fixed those, but then noticed the E of Result<T, E> of the setters is (). That's better replaced with a dedicated type for a couple of reasons, so that's what I implemented. That's a breaking change.

To keep example code from breaking again, I moved it to examples/, which is meaningful to cargo (cargo test will compile those files, so broken code breaks tests). For that guarantee to hold, I removed the duplicate in the README (which might become stale). It's not great to have no sample code in the README though. Let me know what you think.

I also applied a bunch of fixes to lints (Rust and Markdown) throughout, making this PR slightly messy. The commits should be mostly well-behaved though, so we can drop commits/split the PR if you'd like.

anonrig commented 8 months ago

Can you revert the std::error changes since this crate supports no_std?

alexpovel commented 8 months ago

Yeah I saw CI failing and then noticed just all for local development. I gated the Error impl behind the std feature now, as was done for ParseUrlError:

https://github.com/ada-url/rust/blob/be5c26098f3cec1679dbb453d22542c9a7b10902/src/lib.rs#L54

I adjusted the example to demonstrate ergonomics when std is available (? operator) and how to work with it otherwise.

just all now passes locally.

Does that work for you or would you still like that part reverted?

anonrig commented 8 months ago

Does that work for you or would you still like that part reverted?

This seems like a breaking change, and I wonder if it's worth it. Can you revert it for the time being?

alexpovel commented 8 months ago

Ok, reverted it (passes just all).

It's pretty neat to have E of any Result<T, E> be std::error::Error. I agree it's hardly worth a breaking change on its own, but perhaps this can eventually find its way into the API alongside other changes (making it worthwhile).

This PR is now a bit messy (and with less content). I didn't rebase and/or squash in order to keep history so that the implementation can be retrieved later on.

anonrig commented 8 months ago

Thanks for the hard work. I'll merge and release a new version.

anonrig commented 8 months ago

Unfortunately clippy is failing now

alexpovel commented 8 months ago

I'm pretty sure that's unrelated to this PR. It fails at impl_hash_borrow_with_str_and_bytes, which was introduced with Rust 1.76. Locally, I ran 1.75 so didn't see this lint yet.

The last Clippy run on main of this repo succeeded, but was done on 1.75, so didn't have that lint yet either.

Running the equivalent command locally on Rust 1.76 or higher (set channel = "1.76" in rust-toolchain.toml) on current main now fails, too:

$ git clean -xfd && sed -i 's/^channel = .*/channel = "1.76"/' rust-toolchain.toml && cat rust-toolchain.toml && git switch --detach be5c260 && cargo hack clippy --feature-powerset -- -D warnings
Removing target/
[toolchain]
channel = "1.76"
profile = "default"
M       rust-toolchain.toml
HEAD is now at be5c260 build: don't force a specific compiler/version (#60)
info: running `cargo clippy --no-default-features -- -D warnings` on ada-url (1/10)
   Compiling libc v0.2.147
   Compiling proc-macro2 v1.0.66
   Compiling memchr v2.6.2
   Compiling unicode-ident v1.0.11
   Compiling regex-syntax v0.8.2
   Compiling syn v1.0.109
   Compiling link_args v0.6.0
   Compiling convert_case v0.4.0
   Compiling aho-corasick v1.0.5
   Compiling quote v1.0.33
   Compiling jobserver v0.1.26
   Compiling cc v1.0.83
   Compiling regex-automata v0.4.3
   Compiling derive_more v0.99.17
   Compiling regex v1.10.2
   Compiling ada-url v2.2.1 (/home/alex/dev/ada-url-rust)
error: the semantics of `Borrow<T>` around `Hash` can't be satisfied when both `Borrow<str>` and `Borrow<[u8]>` are implemented
   --> src/lib.rs:743:6
    |
743 | impl hash::Hash for Url {
    |      ^^^^^^^^^^
    |
    = note: the `Borrow` semantics require that `Hash` must behave the same for all implementations of Borrow<T>
    = note: however, the hash implementations of a string (`str`) and the bytes of a string `[u8]` do not behave the same ...
    = note: ... as (`hash("abc") != hash("abc".as_bytes())`
    = help: consider either removing one of the  `Borrow` implementations (`Borrow<str>` or `Borrow<[u8]>`) ...
    = help: ... or not implementing `Hash` for this type
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#impl_hash_borrow_with_str_and_bytes
    = note: `#[deny(clippy::impl_hash_borrow_with_str_and_bytes)]` on by default

error: could not compile `ada-url` (lib) due to 1 previous error
error: process didn't exit successfully: `/home/alex/.rustup/toolchains/1.76-x86_64-unknown-linux-gnu/bin/cargo clippy --manifest-path Cargo.toml --no-default-features -- -D warnings` (exit status: 101)

The same command on 1.75 succeeds.

See also https://github.com/rust-lang/rust-clippy/pull/11781 .

We could address this lint, ignore it, fix Rust version to 1.75, ... ?