Remove try-except ValueError to allow UnicodeEncodeError to propagate

[kristang]

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

	try:
	num_packages = len(self._dependency_cache.search_for(dependency))
	except ValueError:
	num_packages = 0

Pull Request Check List

Resolves: #6784

• Added tests for changed code.
• Updated documentation for changed code.

[neersighted]

I did some spelunking; this try-catch was introduced here: 04e1f60#diff-946f8ca4bcf4d43cf2d35b506b3f7a1eded2e9bc7744f089ffb1bc2efdce1673R305-R307

It appears intended to catch ValueError as thrown by the constraints code -- there are still plenty of raise ValueError in poetry.core.constraints. @radoering / @dimbleby, I'd appreciate some eyes on this and thoughts on the risk of this change. It seems like we should refactor the exceptions thrown by poetry-core in general to avoid a generic ValueError (we should probably subclass it) as the fact that it is the superclass of UnicodeError is likely to bite us in the future.

[dimbleby]

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 ValueError is caught a few lines later and if you believe that's unhittable too then lots of code can disappear.

[radoering]

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

It seems like we should refactor the exceptions thrown by poetry-core in general to avoid a generic ValueError (we should probably subclass it)

Nevertheless, if other maintainers want to tread the other path, that's fine with me too.

[dimbleby]

I don't believe this ValueError handling is connected to version or constraint parsing, if it ever was

• the constraint on the dependency that we have in hand has long since been parsed
• versions on the packages that we are finding are handled during the search here
  • and this is how it has to be, else a single badly versioned package would prevent all versions of that package from being installed

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

[kristang]

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)

[dimbleby]

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

[kristang]

Completely fair.

[neersighted]

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.

[kristang]

Should I just close this and let it be replaced by #6790?

[radoering]

Superseded by #6790

[github-actions]

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.