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

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

Merged
merged 9 commits into from
Mar 21, 2024

Conversation

alexpovel
Copy link
Contributor

  • docs: Fix sample code
  • docs: Use unused Result
  • feat!: Error for SetterResult
  • chore: Fix various clippy lints
  • docs: Dedicated examples/
  • docs: Apply Markdown lints

In a new project, after `cargo add ada-url`, now compiles successfully
`set_port` returns a `Result` we `must_use`. Fixes the compiler warning.
This implements `std::error::Error` for a newly
introduced error type. We cannot implement that
trait on `()` in this crate, so cannot offer
consumers convenient error handling via `?` etc.
(`anyhow`, ...) without a dedicated error type we
own.
Found by temporarily adding

```rust
```

to `lib.rs`. Removed it again because not all
lints are solved (I only applied the more useful
ones, which should benefit library consumers at no
costs), and I didn't think putting `#[allow(...)]`
throughout was a good idea.
Files in `examples/` will be compiled by `cargo
test`. Hence, uncompilable code will be caught
from now on.

One can `cargo run --example simple` to see the
`println!` output. This allows users to get
started hacking quickly.
@alexpovel
Copy link
Contributor Author

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.

@alexpovel alexpovel marked this pull request as ready for review March 20, 2024 13:47
@anonrig
Copy link
Member

anonrig commented Mar 20, 2024

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

@alexpovel
Copy link
Contributor Author

alexpovel commented Mar 20, 2024

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:

#[cfg_attr(feature = "std", derive(derive_more::Error))] // error still requires std: https://github.com/rust-lang/rust/issues/103765

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
Copy link
Member

anonrig commented Mar 20, 2024

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
Copy link
Contributor Author

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
Copy link
Member

anonrig commented Mar 21, 2024

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

@anonrig
Copy link
Member

anonrig commented Mar 21, 2024

Unfortunately clippy is failing now

@alexpovel
Copy link
Contributor Author

alexpovel commented Mar 21, 2024

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 rust-lang/rust-clippy#11781 .

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

@anonrig anonrig merged commit 21d1221 into ada-url:main Mar 21, 2024
1 of 6 checks passed
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