Skip to content
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: add missing overload for sub #4

Closed
wants to merge 3 commits into from
Closed

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Oct 4, 2024

add missing overload

class _Regexp(Generic[AnyStr]):
    @overload
    def sub(self, repl: Callable[[_Match[AnyStr]], AnyStr], text: AnyStr, count: int = 0) -> AnyStr: ...

self: _Regexp[str], text: str, pos: int | None = None, endpos: int | None = None
) -> _Match[str] | None: ...
@overload
self, text: AnyStr, pos: int | None = None, endpos: int | None = None
Copy link
Owner

Choose a reason for hiding this comment

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

Context: The point of bytes vs AnyStr in the stubs is to convey the fact that if the input is a str the output will be a Match[str]. If we used AnyStr, then input were str then the output could be either Match[bytes] or Match[str] which loses some typing constraints.

Anyway, happy to just take the missing overload without the AnyStr change if you could scope down this PR to just that.

Copy link
Contributor Author

@trim21 trim21 Oct 4, 2024

Choose a reason for hiding this comment

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

Looks like I misunderstood how re2 handle inputs and you misunderstood how AnyStr works here.

I find that methods on re2.compile(str(...)).match/findall/sub/... can handle bytes input, so it dones't need to be AnyStr. For python stdlib re, pattern and string must have same type but it's not true for re2, re2 can run bytes pattren on string or string pattern on bytes, which means re.findall("1", b"0 1 2") doesn't work but re2.findall("1", b"0 1 2") works.

And AnyStr is a TypeVar and in class _Regexp(Generic[AnyStr]) it will be narrowed base on the type of re2.compile(), but not narrowed at function/method level by the input string.

This make most methods in _Regexp have wrong typing, I'll send a seprated PR to fix them later

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the investigation!

I based the stubs on the idea that re2 would be used as a drop-in for re so I made the constraints stronger than re2 itself supports, but I don't have an issue with making things match the re2 implementation more.

The main issue with that though is since this repo isn't directly related to the re2 one it is easy for the stubs to drift from the implementation in a way that wouldn't be helpful for stub users, but we can discuss that on the next PR if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the in progress stubs produced by the re2 maintainers, google/re2#496, it looks like they are more in line with your original AnyStr change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, which mean using typing from re is fine.

Maybe we can just lock version to google-re2==1.1.* so downstream users will fail to resolve deps if re2 release 1.2, and we can check typing then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the in progress stubs produced by the re2 maintainers, google/re2#496, it looks like they are more in line with your original AnyStr change.

can you point out where the file is? I didn't find it in that issue

Copy link
Contributor Author

@trim21 trim21 Oct 4, 2024

Choose a reason for hiding this comment

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

OK, I find it at https://github.com/google/re2/compare/main...cosmicexplorer:type-stubs?expand=1

But it's in-complete, as I explained before.

checkout #5 please, I have lint error fixed in #5 and I don't want to fix it again here.

@trim21 trim21 changed the title use AnyStr and add missing overload fix: add missing overload for sub Oct 4, 2024
@trim21 trim21 requested a review from ddn0 October 4, 2024 22:07
@trim21 trim21 closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants