Repository navigation
Partially restore old (imprecise) signature of Match.group() - #3190
Conversation
|
@srittau @JelleZijlstra It is not super-urgent, but I would like to ideally get green light from at least one of you today. |
| @overload | ||
| def group(self, __group: Literal[0] = ...) -> AnyStr: ... | ||
| @overload | ||
| def group(self, __group: Union[str, int]) -> Optional[AnyStr]: ... |
There was a problem hiding this comment.
Wouldn't it be sufficient to change this overload and the next to return Any or AnyStr, and keep the first overload the same?
There was a problem hiding this comment.
Or actually if we make it AnyStr, then the two overloads would have the same return type.
What do you think of @srittau's suggestion to make this overload return Any? Returning AnyStr when it can actually return None is also problematic for mypyc.
There was a problem hiding this comment.
Changing this to AnyStr allows to merge the two overloads, but I can keep them separate if you prefer.
There was a problem hiding this comment.
I'd prefer Any for now as well. Also, would a mypy plugin make sense here? Because that is something I could have a stab at.
There was a problem hiding this comment.
What do you think of @srittau's suggestion to make this overload return Any? Returning AnyStr when it can actually return None is also problematic for mypyc.
I am still not sure. My main motivation for AnyStr is that it was like this for years and didn't cause any troubles.
There was a problem hiding this comment.
Also, would a mypy plugin make sense here? Because that is something I could have a stab at.
How exactly would you like to get more precise type with a plugin? In most cases I have seen one can't statically say whether it can be None.
I'd prefer
Anyfor now as well.
Keeping it AnyStr will allow catching many str/bytes/unicode bugs, especially in view of the fact that a lot of code is now migrated to Python 3. As I said, let's solve the problems as they appear. It was AnyStr for years, and everything was OK.
There was a problem hiding this comment.
How exactly would you like to get more precise type with a plugin? In most cases I have seen one can't statically say whether it can be
None.
You can analyze the regex to figure out whether the group is optional.
Keeping it
AnyStrwill allow catching manystr/bytes/unicodebugs, especially in view of the fact that a lot of code is now migrated to Python 3. As I said, let's solve the problems as they appear. It wasAnyStrfor years, and everything was OK.
It did cause some problems:
- Somebody reported Wrong return type for Match.group() #3171
- The current code can lead to crashes with mypyc, What to do about lies in typeshed, such as Match.group mypyc/mypyc#676
There was a problem hiding this comment.
It did cause some problems:
These are not problems: first report appeared nine days ago, second is a discussion of possible crash, not actual one. On the other hand, passing something from bytes match groups to functions accepting strings and vice versa is one of the prime causes of str/unicode bugs.
There was a problem hiding this comment.
Are there any additional arguments? If not, I would proceed with this, because current state does cause problems.
Later we can reconsider.
There was a problem hiding this comment.
OK I'm fine with restoring the previous behavior too.
3 participants
This partially reverts #3172