codecov[bot]
commented
2 months ago Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.63%. Comparing base (
ccd40e1) to head (6ef442b).
Closed jakkdl closed 1 month ago
codecov[bot]
commented
2 months ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.63%. Comparing base (
ccd40e1) to head (6ef442b).
jakkdl
commented
2 months ago Oh, turns out the unrelated typing error does work with pyright even with the overloads, and it's solely a mypy thing. I'll try to get a minimal repro and/or see if it's a known mypy issue and/or report it.
EDIT: fixed by making TypeVar covariant
jakkdl
commented
2 months ago @TeamSpen210 after adding covariant=True to the TypeVar the docs/source/typevars.py fails to resolve it:
/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.RaisesGroup:1: WARNING: py:obj reference target not found: typing.Callable[[BaseExceptionGroup[+E]], bool] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/__init__.py:docstring of trio.testing.Matcher:1: WARNING: py:class reference target not found: type[+E] | None
/home/h/Git/trio/looser_excgroups/src/trio/testing/_raises_group.py:docstring of trio.testing._raises_group._ExceptionInfo.type:1: WARNING: py:class reference target not found: type[+E]I'm a bit surprised as covariant typevars exist in a few other places in the codebase without issue. Unless you have any opinions I'll try do some ugly workaround where I either add a plussed [copy] to the dicts in identify_typevars, or strip +s from the target in lookup_reference
EDIT: nope, wasn't that easy to work around...
jakkdl
commented
2 months ago I can make a theoretical case for splitting strict=False into two separate flags - one that allows unwrapped exceptions, and one that flattens nested exceptiongroups. But I suspect the added burden of having to set both might outweigh the gain from somebody that actually wants them split:
# they want this to pass
with RaisesGroup(ValueError, strict=False):
raise ExceptionGroup("outer", [ExceptionGroup("inner", [ValueError()])])
# but this to fail
with RaisesGroup(ValueError, strict=False):
raise ValueErrorOf course they can just do
with RaisesGroup(..., strict=False) as e:
...
assert isinstance(e, ExceptionGroup)but that's easy to forget and could silently introduce changes in behavior.
One, probably better, case in favor of splitting out allow_unwrapped=True is that we can raise an error in RaisesGroup.__init__ if specifying several exceptions + allow_unwrapped=True. That's both easier to implement and faster to debug for end-users than trying to resolve that with a message on a failed catch https://github.com/python-trio/trio/pull/2989#discussion_r1565979668
Since typing with RaisesGroup(ValueError, allow_unwrapped=True, flatten=True) is a mouthful we (or leave that for pytest) could add convenience aliases, or just rely on end-users to write their own thin wrappers that change the defaults.
TeamSpen210
commented
2 months ago Fixed the typevar-related issues, but now there's something else in history.rst. Not fully sure why the +E is only happening with this typevar, but I found a workaround. By using autodoc_process_signatures() I could insert the fully qualified name for the var, allowing Sphinx to find it.
jakkdl
commented
2 months ago Seems like nobody else had opinions on strict vs allow_unwrapped+flatten_subgroups. The ability to raise a ValueError on users doing RaisesGroup(SyntaxError, TypeError, allow_unwrapped=True) with a helpful message on what else to do convinced me that this is the way to go, even if it will be more verbose when you want to fully emulate except* (i.e. you expect a single exception, but don't care if it's unwrapped, or in any level of nesting).
jakkdl
commented
2 months ago bumping exceptiongroup so whoever reviews the next dep bump don't have to spend any time tracking down why type tests start ""failing"" (i.e. no longer xfail)
jakkdl
commented
2 months ago Looks good. I think I saw a few regexes reading through the changes that could still have end specifiers added, but I think it should be ok.
The missing end specifiers are intended because I didn't bother including the full error message.
I fixed it so RaisesGroup now supports it on ExceptionGroup - previously it failed since it included "1 sub-exception" in the message it matched against, since
>>> str(ExceptionGroup('foo', [ValueError()]))
'foo (1 sub-exception)'oh sorry for the re-request, didn't get notified about your review
jakkdl
commented
1 month ago oh sorry for the re-request, didn't get notified about your review
fixed everything now though, so re-review request is warranted
jakkdl
commented
1 month ago I encountered some issues with typing for functions passed to the check argument, and thought about handling that in a separate pull request - but it got so thorny that I'll just add the failing tests now and maaaybe address it in a separate PR once I hear from https://github.com/python/mypy/issues/17251
A5rocks
commented
1 month ago Also re: your mypy bug, my impression was that it just ignores __new__ in favor of __init__ if it's available.
jakkdl
commented
1 month ago Also re: your mypy bug, my impression was that it just ignores
__new__in favor of__init__if it's available.
scrolling through https://github.com/python/mypy/blob/master/test-data/unit/check-classes.test I guess that is the current state of things with mypy. But after sleeping on it I think I have a better way of resolving it - but that's for a new PR. It's finally time to merge this one :rocket:
strictparameter, renaming it toflatten_subgroups.allow_unwrappedparameter, adding the possibility to more closely mirrorexcept*. This is a feature requested in https://github.com/pytest-dev/pytest/issues/11538#issuecomment-2024361910 by @belm0would fail due to it checking if the number of exceptions was correct before flattening the structure of the raised exceptiongroup.
I also found an unrelated issue where mypy&pyright are not able to deduce the type when passing multipleEDIT: this was due toMatcherobjects matching against different exceptions toRaisesGroup. (orMatcher+class, but two classes work). Fiddled around a little bit but wasn't able to quickly find a fix, and explicitly setting[Exception]isn't especially onerous.TypeVarlacking covariance.