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

Replace failure branch with panic #316

Merged

Conversation

konstin
Copy link
Member

@konstin konstin commented Jan 27, 2025

When the user's implementation breaks the contract about choose version, we now panic. This contract is trivial to enforce (vs.contains(v)), so an error variant only creates an unreachable!() in user code.

Fixes #239

When the user's implementation breaks the contract about choose version, we now panic. This contract is trivial to enforce (`vs.contains(v)`), so an error path does not make sense.

Fixes #239
@konstin konstin requested a review from Eh2406 January 27, 2025 15:33
@Eh2406 Eh2406 merged commit 19c67d0 into konsti/dev/track-priorities-in-a-set Jan 28, 2025
6 checks passed
@Eh2406 Eh2406 deleted the konsti/dev/remove-failure branch January 28, 2025 20:12
@mpizenberg
Copy link
Member

Could we add a hint about potential reasons, and potential ways to diagnostic, fix the problem?

In the past, almost every time someone had an issue with pubgrub, it was because of not strictly abiding by the requirements to implement some trait.

@konstin
Copy link
Member Author

konstin commented Jan 29, 2025

The method name, the expected range and actual version are all in the panic message, is there something more to add?

@mpizenberg
Copy link
Member

mpizenberg commented Jan 29, 2025

Not sure. Maybe something along the lines of "This issue most likely happened because you have a custom DependencyProvider implementation. Please make sure the implementation of the choose_version function is correct, and make sure that you follow all documented requirements for custom dependency providers." With a link to the docs somewhere.

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.

3 participants