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

Remove PubGrubError::Failure #239

Open
konstin opened this issue Jul 15, 2024 · 4 comments
Open

Remove PubGrubError::Failure #239

konstin opened this issue Jul 15, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@konstin
Copy link
Member

konstin commented Jul 15, 2024

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.

@konstin konstin added the enhancement New feature or request label Jul 15, 2024
@mpizenberg
Copy link
Member

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

Eh2406 commented Jul 17, 2024

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

konstin commented Jul 17, 2024

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

Eh2406 commented Jul 17, 2024

Seems reasonable. That all sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants