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

Use a TypeError, rather than a ValueError, when refusing to treat a str as a Vec #2685

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

alex
Copy link
Contributor

@alex alex commented Oct 14, 2022

This is far more consistent with how these exceptions are usually used

@alex alex force-pushed the type-error branch 2 times, most recently from 3dd13aa to 75464db Compare October 14, 2022 01:29
@adamreichold
Copy link
Member

@alex Before we proceed with an API change here, any opinions on #2632 which might remove this code path entirely?

@alex
Copy link
Contributor Author

alex commented Oct 14, 2022

I'm pretty ambivalent.

@alex
Copy link
Contributor Author

alex commented Nov 6, 2022

Given that #2632 hasn't reached a consensus, would it be appropriate to merge this in the event this code path lives on?

…tr as a Vec

This is far more consistent with how these exceptions are usually used
@davidhewitt
Copy link
Member

I think it would make sense to merge this, as it's IMO a reasonable correction over what already exists while we decide whether to merge the bigger change.

@davidhewitt
Copy link
Member

@adamreichold please forgive me if you disagree!

@davidhewitt davidhewitt merged commit 4b7e0ee into PyO3:main Nov 9, 2022
@adamreichold
Copy link
Member

@adamreichold please forgive me if you disagree!

Please don't worry, a disagreement is not an attack, so I don't think there is anything to forgive here.

And while I personally would like #2632 to land, I very much understand that there is no consensus yet and that change should therefore not block reasonable improvements.

@alex alex deleted the type-error branch November 9, 2022 11:56
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