Skip to content

/ cpython

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

bpo-30951: Correct co_names documentation in inspect module #2743

Open
wants to merge 1 commit into
base: master
from
Open

bpo-30951: Correct co_names documentation in inspect module #2743

wants to merge 1 commit into from
+1 −1

Conversation

@jalexvig
Copy link

jalexvig commented Jul 17, 2017 •
edited by bedevere-bot

Previously co_names was described as containing names of local variables. co_names however contains names of global variables (co_varnames contains local variable names). Documentation was updated to reflect this.

Relevant stackoverflow post:

https://stackoverflow.com/questions/45147260/what-is-co-names

https://bugs.python.org/issue30951
@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Jul 17, 2017

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!
@mention-bot
Copy link

mention-bot commented Jul 17, 2017

@jalexvig, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Yhg1s, @1st1 and @zestyping to be potential reviewers.
@jalexvig jalexvig changed the title Correct co_names documentation in inspect module bpo-30951: Correct co_names documentation in inspect module Jul 17, 2017
@marco-buttu
marco-buttu reviewed Jul 18, 2017
View changes
Lib/inspect.py Outdated
@@ -268,7 +268,7 @@ def iscode(object):
co_kwonlyargcount number of keyword only arguments (not including ** arg)
co_lnotab encoded mapping of line numbers to bytecode indices
co_name name with which this code object was defined
co_names tuple of names of local variables
co_names tuple of names of global variables

This comment has been minimized.

Sign in to view
Copy link
@marco-buttu

marco-buttu Jul 18, 2017

Contributor

What about tuple of names of global variables used in the bytecode?

This comment has been minimized.

Sign in to view
Copy link
@jalexvig

jalexvig Jul 20, 2017

Author

This description is currently under the code type... The other descriptions don't make explicit reference to bytecode (except for co_consts) since it is implied by the code type. Would be pretty repetitive to add reference to the bytecode in every description. Thoughts?

This comment has been minimized.

Sign in to view
Copy link
@bitdancer

bitdancer Jul 20, 2017

Member

The difference here is that "tuple of names of global variables" would be potentially misleading, since only those global variables used by the bytecode are included. (IIUC) This item is parallel to co_consts in that sense.

This comment has been minimized.

Sign in to view
Copy link
@marco-buttu

marco-buttu Jul 20, 2017

Contributor

Thanks @bitdancer, you pointed out perfectly my concern :-)

This comment has been minimized.

Sign in to view
Copy link
@jalexvig

jalexvig Jul 20, 2017

Author

Ah gotcha - updating.
@jalexvig jalexvig force-pushed the jalexvig:fix-issue-30951 branch from fc5a4bc to 17cb727 Jul 20, 2017
@bitdancer
bitdancer approved these changes Jul 20, 2017
View changes
@Mariatta Mariatta closed this Jul 5, 2018
@Mariatta Mariatta reopened this Jul 5, 2018
@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 23, 2018
Closed
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Nov 23, 2018

co_names contains not only names of global variables. See my comment on the tracker.
@csabella
Copy link
Contributor

csabella commented May 17, 2019

@jalexvig, please take a look at incorporating the note from @serhiy-storchaka in your change. Thanks!
@auvipy
auvipy approved these changes May 21, 2019
View changes
@csabella csabella added the stale label Jan 20, 2020
@csabella
Copy link
Contributor

csabella commented Jan 20, 2020

Added the stale label as there hasn't been any response to the review comments.
@laike9m
Copy link
Contributor

laike9m commented Mar 23, 2020

So I'm guessing, co_names == all names in code - co_varnames - co_freevars ?
@smarie smarie mentioned this pull request Mar 23, 2020
Open
@csabella
Copy link
Contributor

csabella commented May 23, 2020

@jalexvig ping
@laike9m
Copy link
Contributor

laike9m commented Jul 23, 2020

Kindly ping
@jalexvig jalexvig force-pushed the jalexvig:fix-issue-30951 branch from 17cb727 to 960d871 Aug 18, 2020
@jalexvig Alex Vig
Correct co_names documentation in inspect module
Loading status checks…
a223eca
@jalexvig jalexvig force-pushed the jalexvig:fix-issue-30951 branch from 960d871 to a223eca Aug 18, 2020
@jalexvig
Copy link
Author

jalexvig commented Aug 18, 2020

I haven't looked at this in a while, but updated with suggestion from Xavier Morel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-buttu marco-buttu

@bitdancer bitdancer

@auvipy auvipy

Assignees
No one assigned
Labels
CLA signed awaiting merge stale
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.