GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-122575: Include `sys.flags.gil` as a sequence attribute by colesbury · Pull Request #122576 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Aug 1, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Make sure that sys.flags.gil is included when sys.flags is treated as a tuple or named tuple.

  • Issue: sys.flags.gil should be a "sequence" attribute OR stop adding to the sequence #122575

📚 Documentation preview 📚: https://cpython-previews--122576.org.readthedocs.build/en/122576/library/sys.html#sys.flags

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
pythongh-122575: Include sys.flags.gil as a sequence attribute
cfaa04f 
Make sure that `sys.flags.gil` is included when `sys.flags` is treated
as a tuple or named tuple.
@bedevere-app bedevere-app Bot mentioned this pull request Aug 1, 2024
sys.flags.gil should be a "sequence" attribute OR stop adding to the sequence #122575
Closed
@itamaro itamaro added the needs backport to 3.13 only security fixes label Aug 1, 2024
@colesbury
sys.flags.gil may be None or an int
7a41cfc
@colesbury
colesbury marked this pull request as ready for review August 1, 2024 20:10
@colesbury
colesbury requested a review from ericsnowcurrently as a code owner August 1, 2024 20:10
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 1, 2024
@colesbury

colesbury commented Aug 1, 2024

Copy link
Copy Markdown
Contributor Author

@ericsnowcurrently, when you have some time, I'd appreciate your review on this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Aug 2, 2024
View reviewed changes
Comment thread Python/sysmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/sys.rst
* - .. attribute:: flags.warn_default_encoding
- :option:`-X warn_default_encoding <-X>`

* - .. attribute:: flags.gil

@picnixz picnixz Aug 2, 2024

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.

You should explain that the gil takes values 0, 1, or 2 and possibly explain the corresponding values (or a link to them) (and also say that None means the free-threaded build (if I'm not wrong)).

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Aug 9, 2024

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.

This isn't the right place for that, is it?

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Aug 10, 2024

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.

Oh actually I don't really know where it would be the right place... I just wanted to point it out since a new variable is being documented (I don't know whether there already exists another place so feel free to ignore this comment!)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_sys.py
attr_types = {
"dev_mode": bool,
"safe_path": bool,
"gil": (int, type(None)),

@picnixz picnixz Aug 2, 2024 •
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.

Does the None value mean that it's the free-threaded build or is it for something else?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AA-Turner

AA-Turner commented Aug 3, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Potentially,

 .. data:: flags
    The :term:`named tuple` *flags* exposes the status of command line
    flags. The attributes are read only.
+   The number of flags may change in any version of Python,
+   do not unpack ``sys.flags`` or index into it as a tuple.

which would document that we change sys.flags with some regularity.

A
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently
ericsnowcurrently approved these changes Aug 9, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Aug 9, 2024
vstinner
vstinner approved these changes Sep 2, 2024
View reviewed changes

@vstinner vstinner 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 if you apply the Py_ARRAY_LENGTH() suggestion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury @picnixz
Update Python/sysmodule.c
dbf5189 
Co-authored-by: Bénédikt Tran <[email protected]>
@vstinner

vstinner commented Sep 2, 2024

Copy link
Copy Markdown
Member

Ah, you should now update test_pythontypes() of test_sys.
All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

I'm going to close this for now as @encukou expressed concerns about backwards compatibility on the issue: #122575 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury colesbury closed this Oct 15, 2024
@colesbury colesbury removed the needs backport to 3.13 only security fixes label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments
@vstinner vstinner vstinner approved these changes
@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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.

6 participants

@colesbury @AA-Turner @vstinner @ericsnowcurrently @picnixz @itamaro
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 01402503 — 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.