Skip to content

Partially restore old (imprecise) signature of Match.group() #3190

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.

Sign up for GitHub

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

Merged
ilevkivskyi merged 1 commit into from
Aug 16, 2019
Merged

Partially restore old (imprecise) signature of Match.group() #3190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions stdlib/3/typing.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ from abc import abstractmethod, ABCMeta
from types import CodeType, FrameType, TracebackType
import collections # Needed by aliases like DefaultDict, see mypy issue 2986

if sys.version_info < (3, 8):
from typing_extensions import Literal

# Definitions of special type checking related constructs. Their definition
# are not used, so their value does not matter.

Expand Down Expand Up @@ -528,17 +525,16 @@ class Match(Generic[AnyStr]):

def expand(self, template: AnyStr) -> AnyStr: ...

# TODO: The return for a group may be None, except if __group is 0 or not given.
@overload
def group(self, __group: Literal[0] = ...) -> AnyStr: ...
@overload
def group(self, __group: Union[str, int]) -> Optional[AnyStr]: ...

@JelleZijlstra JelleZijlstra Aug 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to change this overload and the next to return Any or AnyStr, and keep the first overload the same?

@JelleZijlstra JelleZijlstra Aug 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilevkivskyi ilevkivskyi Aug 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to AnyStr allows to merge the two overloads, but I can keep them separate if you prefer.

@srittau srittau Aug 16, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilevkivskyi ilevkivskyi Aug 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilevkivskyi ilevkivskyi Aug 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Any for 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.

@JelleZijlstra JelleZijlstra Aug 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

It did cause some problems:

@ilevkivskyi ilevkivskyi Aug 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilevkivskyi ilevkivskyi Aug 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any additional arguments? If not, I would proceed with this, because current state does cause problems.

Later we can reconsider.

@JelleZijlstra JelleZijlstra Aug 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'm fine with restoring the previous behavior too.

def group(self, __group: Union[str, int] = ...) -> AnyStr: ...
@overload
def group(
self,
__group1: Union[str, int],
__group2: Union[str, int],
*groups: Union[str, int],
) -> Tuple[Optional[AnyStr], ...]: ...
) -> Tuple[AnyStr, ...]: ...

def groups(self, default: AnyStr = ...) -> Sequence[AnyStr]: ...
def groupdict(self, default: AnyStr = ...) -> dict[str, AnyStr]: ...
Expand Down