search

pubgrub-rs / pubgrub

PubGrub version solving algorithm implemented in Rust
https://pubgrub-rs.github.io/pubgrub/pubgrub/
Mozilla Public License 2.0
356 stars 32 forks source link

Remove `PubGrubError::Failure` #239

Open konstin opened 1 month ago

konstin commented 1 month ago

PubGrubError currently has a PubGrubError::Failure(String) variant with two usages: "a package was chosen but we don't have a term." and "choose_package_version picked an incompatible version". Both of these only occur when breaking the dependency provider contract. We should either replace them by panics, or replace the String with two dedicated variants.

mpizenberg commented 1 month ago

choose_package_version is also an old name based on the v0.2 API.

I don’t have strong opinions on this. I think having a failure type instead of panics can provide better error messages since it can be unwind at the most adequate level to provide useful information as to why and in which context that error occurred.

Eh2406 commented 1 month ago

Looking more closely at both examples, they should clearly be panics. For the first one it's not even clear how this could be a user's fault, something in the algorithm has gone very wrong if we end up in the situation. For the second one, the user has clearly messed up, but it's not hard to provide all of the context namely what package was requested with what VS and what V was returned.

On the other hand, this provided us a way to add new errors without making a breaking change. If that something we care about we should mark this enum as "nonexhaustive".

konstin commented 1 month ago

As a user, i'd like to handle pubgrub errors exhaustively and if required match on or add context to DependencyProvider::Err, so i'd new variants to be breaking changes.

On a related note, in uv we removed PubGrubError::SelfDependency since in python sometimes packages depend on themselves since pip allows it, it's just a no-op.

Eh2406 commented 1 month ago

Seems reasonable. That all sounds good.