-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 try-except ValueError to allow UnicodeEncodeError to propagate #6786
Remove try-except ValueError to allow UnicodeEncodeError to propagate #6786
Conversation
UnicodeEncodeError is a subclass of UnicodeError and thus a subclass of ValueError.
I did some spelunking; this try-catch was introduced here: 04e1f60#diff-946f8ca4bcf4d43cf2d35b506b3f7a1eded2e9bc7744f089ffb1bc2efdce1673R305-R307 It appears intended to catch |
I mean 2018... whatever! This change was at my suggestion so I'm just doubling down rather than adding new opinion, but I'm good with this. If and when reports roll in of things that this used to catch then they can be fixed: but that the poetry test suite does not hit any reassures me that this at worst very far from the main line. Actually once you believe in this MR you might as well go further, the same |
If it's from 2018 and it's not clearly evident that a test was added these days, I'm not very confident that there is something in the test suite. On the one hand, removing it is the best way to get feedback. On the other hand, it's quite disruptive. Since I'm normally a cautious person I'd probably go with
Nevertheless, if other maintainers want to tread the other path, that's fine with me too. |
I don't believe this ValueError handling is connected to version or constraint parsing, if it ever was
By all means tighten up the exceptions in poetry-core and elsewhere, but I do not think this fix needs to be blocked on that |
Would the pseudo-fix I suggested in the issue I created be more palatable? I'm not familiar enough with your development practices, but I understand the hesitation with just up and removing a try-except. if credential.username is not None or credential.password is not None:
try:
request = requests.auth.HTTPBasicAuth(
credential.username or "", credential.password or ""
)(request)
except UnicodeEncodeError as e:
raise PoetryException(e) |
IMO that's a step in the wrong direction rather than a step in the right direction. The current try-except is either too broad or - as I contend - altogether unwanted, and working around that by cluttering up the rest of the code with this sort of thing is undesirable |
Completely fair. |
I'm inclined to go with @dimbleby's version as I don't see any evidence that the try/catch in this code is doing anything useful. I do think that we should open an issue to subclass exceptions in poetry-core and make them more specific. |
Should I just close this and let it be replaced by #6790? |
Superseded by #6790 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
UnicodeEncodeError is currently silently caught and not passed through to end-user when encountered.
UnicodeEncodeError is a subclass of UnicodeError and thus a subclass of ValueError which is currently caught in try-except ValueError in
mixology/version_solver.py
:poetry/src/poetry/mixology/version_solver.py
Lines 398 to 401 in 0ffe91c
Pull Request Check List
Resolves: #6784