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 type annotation of pstats.FunctionProfile.ncalls #96741

Merged

Conversation

ruancomelli
Copy link
Contributor

This PR aligns the type annotation of pstats.FunctionProfile.ncalls with its runtime str type.

The fact that the runtime type of pstats.FunctionProfile.ncalls is str can be verified by (1) inspecting the source code for pstats and (2) running a small test snippet:

  1. in the source code for pstats, the only place where ncalls is defined seems to be

    ncalls = str(nc) if nc == cc else (str(nc) + '/' + str(cc))
    where the assigned value is always a string;

  2. you can also check that stats.FunctionProfile.ncalls is a string by running the following script:

    from cProfile import Profile
    from pstats import Stats
    
    # execute random code to generate profile stats
    with Profile() as profile:
        print("foo")
    
    # get a sample function profile
    stats_profile = Stats(profile).get_stats_profile()
    function_profile = next(iter(stats_profile.func_profiles.values()))
    
    # check that the type of `function_profile.ncalls` is indeed `str`
    assert type(function_profile.ncalls) is str

I also have an open PR in typeshed to fix this issue there: python/typeshed#8712.

This change seems small and straightforward enough not to require an issue, but I am happy to open one if you think it is necessary. I also did not include any tests since this is a type annotation fix without any changes to the runtime behavior.

This change aligns the type annotation of `pstats.FunctionProfile.ncalls` with its runtime type.
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Sep 10, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM!

This change seems small and straightforward enough not to require an issue, but I am happy to open one if you think it is necessary. I also did not include any tests since this is a type annotation fix without any changes to the runtime behavior.

I agree that this probably doesn't need an issue or NEWS entry, since there's no user-visible change introduced by this PR. (Type checkers use information from typeshed for the stdlib, rather than looking at any annotations in the CPython codebase.) I think the change is still valuable, however, since it's highly confusing for readers of the code for there to be an incorrect annotation here.

I don't really have an opinion on whether this should be backported or not -- on the one hand, there's no user-visible change here; on the other hand, there's no real risk to it being backported.

@AlexWaygood
Copy link
Member

@gpshead, as the person who reviewed and merged #15495, which introduced this class -- does this look okay to you?

@gpshead
Copy link
Member

gpshead commented Sep 14, 2022

It's still useful to add a NEWS entry saying "corrected the type annotation on data class blah.blah.thing to be str."

@gpshead gpshead added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes and removed skip news labels Sep 14, 2022
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ruancomelli
Copy link
Contributor Author

Hi @gpshead, thanks for the suggestion. I added a NEWS file using blurb_it.
Since there is no GitHub issue linked to this PR, and IDs are unique for PRs and issues, I used this PR's ID 96741 when generating the NEWS file. Please let me know if this somehow breaks the usual merging workflow.

@gpshead gpshead merged commit 8e9a37d into python:main Sep 15, 2022
@miss-islington
Copy link
Contributor

Thanks @ruancomelli for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-96835 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed needs backport to 3.11 only security fixes needs backport to 3.10 only security fixes labels Sep 15, 2022
@bedevere-bot
Copy link

GH-96836 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 15, 2022
* fix: annotate `pstats.FunctionProfile.ncalls` as `str`

This change aligns the type annotation of `pstats.FunctionProfile.ncalls` with its runtime type.
(cherry picked from commit 8e9a37d)

Co-authored-by: Ruan Comelli <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 15, 2022
* fix: annotate `pstats.FunctionProfile.ncalls` as `str`

This change aligns the type annotation of `pstats.FunctionProfile.ncalls` with its runtime type.
(cherry picked from commit 8e9a37d)

Co-authored-by: Ruan Comelli <[email protected]>
AlexWaygood added a commit to python/typeshed that referenced this pull request Sep 15, 2022
The annotation in CPython was fixed thanks to @ruancomelli in python/cpython#96741!
@ruancomelli ruancomelli deleted the fix-pstats-functionprofile-ncalls-annotation branch September 15, 2022 10:20
AlexWaygood added a commit to python/typeshed that referenced this pull request Sep 15, 2022
The annotation in CPython was fixed thanks to @ruancomelli in python/cpython#96741!
ambv pushed a commit that referenced this pull request Oct 5, 2022
…) (#96835)

This change aligns the type annotation of `pstats.FunctionProfile.ncalls` with its runtime type.
(cherry picked from commit 8e9a37d)

Co-authored-by: Ruan Comelli <[email protected]>
ambv pushed a commit that referenced this pull request Oct 5, 2022
…) (#96836)

This change aligns the type annotation of `pstats.FunctionProfile.ncalls` with its runtime type.
(cherry picked from commit 8e9a37d)

Co-authored-by: Ruan Comelli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants