GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-120029: make `symtable.Symbol.__repr__` correctly reflect the compiler's flags by picnixz · Pull Request #120099 · python/cpython

Conversation

picnixz commented Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

I did not expose is_free_class yet because I don't really know whether it's the best name or not, nor did I handle the DEF_FREE flag because I don't know its difference with the FREE scope itself (see #120029 (comment)).

I've nonethless cleaned up the lambda functions used to getting the scope and also exported SCOPE_OFFSET instead of SCOPE_OFF (those constants are actually private since they are not documented so it should not affect public API). I can revert those changes easily though if you want me to make them in another PR.

cc @JelleZijlstra @carljm

Issue: Some flags are not publicly exported by symtablemodule.c #120029

📚 Documentation preview 📚: https://cpython-previews--120099.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

picnixz added 8 commits

June 5, 2024 10:58


          export more flags

ecf13fa


          rename private module constant "SCOPE_OFF" -> "SCOPE_OFFSET"

6b52473


          add private Python function mirroring '_PyST_GetScope'

4fe8b34


          fix some reference in a comment

73f8dc7


          handle exported flags

477ad7f


          add test

d04b013


          add doc

ff02dfa


          blurb

5c612de

bedevere-app Bot added the awaiting review label

Jun 5, 2024

bedevere-app Bot mentioned this pull request

Jun 5, 2024

Some flags are not publicly exported by symtablemodule.c #120029

Closed

JelleZijlstra self-requested a review

June 5, 2024 13:26

serhiy-storchaka reviewed

Jun 5, 2024

View reviewed changes

Comment thread

Lib/symtable.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/symtable.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          address review

1e84973

- restore 'symtable.SCOPE_OFF' for compatibility purposes
- fix `Symbol.is_comp_cell()`

picnixz commented Jun 6, 2024

Copy link

Copy Markdown

Member Author

Thank you for the review Serhiy. By the way, is it fine for me to re-request a review after addressing what you've reviewed or do you prefer not to have notifications' noise (same question for Jelle)?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 2 commits

June 6, 2024 10:47


          revert to using SCOPE_OFF

388e40e


          fixup new line

9a74d6b

serhiy-storchaka approved these changes

Jun 6, 2024

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz reacted with hooray emoji

All reactions

🎉 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting review labels

Jun 6, 2024

carljm requested changes

Jun 6, 2024

View reviewed changes

carljm left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

DEF_FREE_CLASS is used in a case like this:

def f():
    x = 1
        class C:
            x = 2
            def method(self):
                return x

The method() method should return 1 -- it doesn't see the class-scoped x, but it does see the function-scoped x outside the class. The class-scoped x is marked DEF_FREE_CLASS in this case, to make that work.

It's such a niche use that I don't think we are going to find a method name here that nicely represents it in an intuitive way. Best option is probably just is_free_class to match the flag name.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/symtable.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added awaiting changes and removed awaiting merge labels

Jun 6, 2024

bedevere-app Bot commented Jun 6, 2024

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 5 commits

June 7, 2024 09:11


          expose is_free_class

46f1604


          un-expose DEF_FREE

f853905


          re-blurb

f2ef899


          add test

c915f04


          fix lint

d101a58

picnixz requested a review from carljm

June 7, 2024 07:27

picnixz commented Jun 7, 2024

Copy link

Copy Markdown

Member Author

Ah, I should have said "I have made the requested changes; please review again".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added the awaiting change review label

Jun 7, 2024

bedevere-app Bot commented Jun 7, 2024

Copy link

Copy Markdown

Thanks for making the requested changes!

@carljm, @serhiy-storchaka: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot requested a review from serhiy-storchaka

June 7, 2024 08:11

serhiy-storchaka reviewed

Jun 7, 2024

View reviewed changes

Comment thread

Doc/library/symtable.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Library/2024-06-05-11-03-10.gh-issue-120029.QBsw47.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 5 commits

June 7, 2024 11:47


          fix indentation

6c450fa


          update NEWS

c44622b


          update CHANGELOG

379c368


          fix typo


          fixup

ede2a58

JelleZijlstra reviewed

Jun 7, 2024

View reviewed changes

Comment thread

Doc/library/symtable.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 2 commits

June 7, 2024 13:42


          address review

9d8dfb4


          fixup

de459bb

JelleZijlstra reviewed

Jun 7, 2024

View reviewed changes

Comment thread

Doc/library/symtable.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

carljm approved these changes

Jun 11, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting change review labels

Jun 11, 2024


          Merge branch 'main' into export-symtable-flags

5e569ec

carljm enabled auto-merge (squash)

June 11, 2024 14:49

carljm disabled auto-merge

June 11, 2024 15:17

carljm commented Jun 11, 2024

Copy link

Copy Markdown

Member

I think this now needs to be updated for compatibility with the merge of the PEP 649 compiler implementation.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz added 2 commits

June 12, 2024 11:59


          Merge branch 'main' into export-symtable-flags

2cbae12


          fix test

ac51d69

serhiy-storchaka reviewed

Jun 12, 2024

View reviewed changes

Comment thread

Lib/test/test_symtable.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra approved these changes

Jun 12, 2024

View reviewed changes

JelleZijlstra merged commit 755dab7 into python:main

Jun 12, 2024

bedevere-app Bot removed the awaiting merge label

Jun 12, 2024

picnixz deleted the export-symtable-flags branch

June 12, 2024 11:17

picnixz mentioned this pull request

Jun 22, 2024

Update symtable stubs for 3.13 and 3.14 python/typeshed#12183

Merged

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request

Jun 30, 2024


          pythongh-120029: make symtable.Symbol.__repr__ correctly reflect th…

c7091e5

…e compiler's flags, add methods (python#120099)

Expose :class:`symtable.Symbol` methods :meth:`~symtable.Symbol.is_free_class`,
:meth:`~symtable.Symbol.is_comp_iter` and :meth:`~symtable.Symbol.is_comp_cell`.

---------

Co-authored-by: Carl Meyer <[email protected]>

estyxx pushed a commit to estyxx/cpython that referenced this pull request

Jul 17, 2024


          pythongh-120029: make symtable.Symbol.__repr__ correctly reflect th…

620b931

…e compiler's flags, add methods (python#120099)

Expose :class:`symtable.Symbol` methods :meth:`~symtable.Symbol.is_free_class`,
:meth:`~symtable.Symbol.is_comp_iter` and :meth:`~symtable.Symbol.is_comp_cell`.

---------

Co-authored-by: Carl Meyer <[email protected]>

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

carljm

carljm approved these changes

JelleZijlstra

JelleZijlstra approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

picnixz commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz commented Jun 6, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

carljm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-app Bot commented Jun 6, 2024

Uh oh!

picnixz commented Jun 7, 2024

Uh oh!

bedevere-app Bot commented Jun 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm commented Jun 11, 2024

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

picnixz commented Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

carljm left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.