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.

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

Jump to bottom

Type annotations on md5, sha1, and sha256 should be overloaded #2046

Open
samueljsb opened this issue May 13, 2024 · 6 comments · May be fixed by #2047
Open

Type annotations on md5, sha1, and sha256 should be overloaded #2046

samueljsb opened this issue May 13, 2024 · 6 comments · May be fixed by #2047

Comments

@samueljsb
Copy link

  • Faker version: 25.0.1
  • OS: all

The docstrings for these functions state:

If raw_output is False (default), a hexadecimal string representation of the MD5 hash
will be returned. If True, a bytes object representation will be returned instead.

But the type annotations do not reflect this:

$ cat t.py
import faker

fake = faker.Faker()

x = fake.md5(raw_output=False)
reveal_type(x)

y = fake.md5(raw_output=True)
reveal_type(y)

$ mypy t.py
$ mypy t.py
t.py:6: note: Revealed type is "Union[builtins.bytes, builtins.str]"
t.py:9: note: Revealed type is "Union[builtins.bytes, builtins.str]"
Success: no issues found in 1 source file

The revealed types ought to be builtins.str and builtins.bytes respectively.

I believe this can be achieved by overloading the type annotations on those methods (using typing.overload.

I would be happy to open a PR to make those changes, if that would be welcome?
@fcurella
Copy link
Collaborator

Thank you so much, that would be wonderful!

@samueljsb
Copy link
Author

samueljsb commented May 15, 2024 •
edited

I've tried to do this but I'm having trouble with the stub files. They don't seem to match the type annotations I put in the actual Python modules (even after running the stub generation script) and manually editing them gets undone when I run the linters.

EDIT: the stub generation doesn't appear to be running in CI, so that isn't a blocker, but it does mean we run the risk of this regressing.

samueljsb added a commit to samueljsb/faker that referenced this issue May 15, 2024
@samueljsb
Overload hash generation functions to make return types more specific
a8f6707 
Before this change, the return type for these functions was always
`bytes| str`. However, the type can be narrowed based on the value
passed to `raw_output`.

This change adds overload annotations for the two allowable values to
allow type checkers to narrow the type and correctly infer the exact
return type.

Resolves joke2k#2046
@samueljsb samueljsb linked a pull request May 15, 2024 that will close this issue
Open
@steve-mavens
Copy link

steve-mavens commented May 17, 2024 •
edited

If I'm still in time to catch this train - the same applies to uuid4(), which has a return type of str | bytes | UUID, and the actual type returned is similarly controlled by a function parameter. #2042

@fcurella
Copy link
Collaborator

fcurella commented Jun 3, 2024

the generate_stubs.py script will need to be updated to detect overloads, but typing.get_overloads() is only available in Python 3.11, and we need to support at least 3.8.

@steve-mavens
Copy link

It's in typing_extensions if that's any help for pre-3.11. The trouble is it that pre-3.11 it only detects overloads defined using typing_extensions.overload, not typing.overload.

@fcurella
Copy link
Collaborator

fcurella commented Jun 3, 2024

I dont see any issues with falling back to typing_extension on <3.11. Do you want to try to update generate_stubs.py to use get_overloads()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Overload hash generation functions to make return types more specific samueljsb/faker
3 participants
@fcurella @samueljsb @steve-mavens