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

stubgen: Preserve simple defaults in function signatures #15355

Merged
merged 9 commits into from Nov 27, 2023

Conversation

hamdanal
Copy link
Contributor

@hamdanal hamdanal commented Jun 2, 2023

@hamdanal
Copy link
Contributor Author

hamdanal commented Jun 2, 2023

Some questions for Alex and Jelle:

  1. Do you want full parity with stubdefaulter?
  2. Should I implement built in container types? (added)
  3. What about name expression default like def f(x=dict)?
  4. I left a todo in the code about what should be the length limit for a "simple default", currently it is 200 characters. What do you think.

@hamdanal hamdanal marked this pull request as ready for review June 7, 2023 14:09
@hamdanal
Copy link
Contributor Author

hamdanal commented Jun 7, 2023

I installed the following packages

matplotlib==3.7.1
numpy==1.24.3
pandas==2.0.2
scipy==1.10.1
seaborn==0.12.2

and generated their stubs with mypy master then added defaults with stubdefaulter then compared the result with defaults added by this PR. The only differences I saw were related to the runtime vs static nature of the two tools:

  1. stubdefaulter not being able to add defaults to a file due to runtime errors, stubgen defaults were added
  2. if a default is set to a module variable, stubdefaulter uses its value as the default while stubgen ignores it

Please let me know if you have other ideas to test this change.

@coditamar
Copy link

@CodiumAI-Agent please review

@CodiumAI-Agent

This comment was marked as spam.

@hauntsaninja
Copy link
Collaborator

@coditamar Please stop, or I'll have to report your account

@coditamar
Copy link

coditamar commented Jul 7, 2023

@coditamar Please stop, or I'll have to report your account

OK. I apologize. thank you

@hoechenberger
Copy link

Hello! May I ask what's the status of this? 🙂 thank you!

@hamdanal
Copy link
Contributor Author

Hello! May I ask what's the status of this? 🙂 thank you!

I don’t have an answer for you but if you need default values in stub files now I invite you to try https://github.com/JelleZijlstra/stubdefaulter. I use it regularly and it works very well.

if rvalue.name in ("None", "True", "False"):
return rvalue.name, True
elif isinstance(rvalue, (IntExpr, FloatExpr)):
return f"{rvalue.value}", True
Copy link
Member

Choose a reason for hiding this comment

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

Would this work correctly for NaN and infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this work correctly for NaN and infinity?

Do these have a literal syntax? float("nan") is a call expression that is ignored when used as a default value in the current implementation. I'll add a test.

[out]
def f(x: bytes = ...) -> None: ...
def f(x: bytes = b'foo', y: bytes = b"what's up") -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Can you add tests for bytes containing non-ASCII characters? Not convinced that would be handled correctly.

This comment has been minimized.

mypy/stubdoc.py Outdated Show resolved Hide resolved
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

discord.py (https://github.com/Rapptz/discord.py): typechecking got 1.07x faster (189.3s -> 177.3s)
(Performance measurements are based on a single noisy sample)

@JelleZijlstra JelleZijlstra merged commit e69c5cd into python:master Nov 27, 2023
19 checks passed
@hamdanal hamdanal deleted the stubgen-defaults branch November 27, 2023 05:58
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.

stubgen drops default values for function arguments
6 participants