-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix Match.group problems #3172
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
Fix Match.group problems #3172
Conversation
* group() may return None values * group() allows to mix int and str arguments * Use concrete return type * Merge overloads
Why do you say it is "clearly wrong" to return AnyStr? As far as I can tell, the only problem with that is that we don't handle the Could you also make the arguments positional-only?
|
Well, "clearly wrong" -> "not type-safe in the general case". Will change them to positional only. |
@@ -525,10 +525,16 @@ class Match(Generic[AnyStr]): | |||
|
|||
def expand(self, template: AnyStr) -> AnyStr: ... | |||
|
|||
def group(self, __group: Literal[0] = ...) -> AnyStr: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the @overload
decorator
This causes 19 errors in mypy self-check and 45 errors in the Python 3 part of our internal code base (which is actually relatively small). TBH, I am not sure the |
I wonder why our tests did not catch this, since the mypy self-test is run as part of the build. I'm open to changing it back, although just removing if m.group(12) is None:
... While I don't think such a tool exists yet, I'd prefer to use |
Actually I was wrong about number of errors internally, it is not 45 it is 110. So I think it makes sense reverting this in some form. As for the |
This partially reverts #3172
Fixes #3171
Returning
Optional
is potentially problematic. It is often obvious from a regexp whether a certain group exists (every time it is non-optional). Many users may rely ongroup()
not returning anOptional
. Nevertheless, returningAnyStr
is clearly wrong. One alternative is returningAny
, but that would lose any type checking ability. Personally, I believe this patch is the best solution, despite the problems it may cause.