Skip to content

bpo-41790: update error in documentation - #22242

Open
koubaa wants to merge 1 commit into
python:mainfrom
koubaa:bpo-1635741-doc
Open

bpo-41790: update error in documentation#22242
koubaa wants to merge 1 commit into
python:mainfrom
koubaa:bpo-1635741-doc

Conversation

@koubaa

@koubaakoubaa commented Sep 14, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

@bedevere-botbedevere-bot added docs Documentation in the Doc dir awaiting review labels Sep 14, 2020
@koubaa

koubaa commented Sep 14, 2020

Copy link
Copy Markdown
ContributorAuthor

@vstinner@corona10@shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

@vstinner

vstinner commented Sep 14, 2020

Copy link
Copy Markdown
Member

@vstinner@corona10@shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

You have the wrong issue number.

@koubaakoubaa changed the title bpo-1635741: update error in documentationbpo-41790: update error in documentationSep 15, 2020
@koubaa

koubaa commented Sep 15, 2020

Copy link
Copy Markdown
ContributorAuthor

@vstinner@corona10@shihai1991 I'm sure I have the wrong issue number but wanted to propose this change to the doc. I can change the number and rebase.

You have the wrong issue number.

@vstinner I made a new issue in that case.

vstinner
vstinner reviewed Sep 15, 2020
View reviewed changes
Comment threadMisc/NEWS.d/next/Core and Builtins/2020-09-14-13-40-18.bpo-1635741.o6PrOV.rst Outdated
eamanu
eamanu approved these changes Sep 16, 2020
View reviewed changes

@eamanueamanu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@koubaa

koubaa commented Sep 20, 2020

Copy link
Copy Markdown
ContributorAuthor

@vstinner is anything else needed before merging this?

@koubaa

koubaa commented Nov 15, 2020

Copy link
Copy Markdown
ContributorAuthor

@vstinner bump

vstinner
vstinner reviewed Nov 16, 2020
View reviewed changes
Comment threadDoc/c-api/intro.rst
declared. The sole exception are the type objects; since these must never be
deallocated, they are typically static :c:type:`PyTypeObject` objects.
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.

@vstinnervstinnerNov 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"The sole exception are the type objects" looks wrong to me. Almost all types in Python are allocated on the heap.

Only static types are never deallocated.

I'm not sure why these kinds of technical details are giving in an introduction. It would be better to move them to https://docs.python.org/dev/c-api/refcounting.html no?

@vstinnervstinnerNov 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or at least, copied there.

@shihai1991shihai1991Nov 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I am not find the docs of PyHeapTypeObject. Maybe add those description info in it.

@erlend-aaslanderlend-aaslandAug 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not certain what to do with this change. The whole paragraph could be revisited, as Victor hints to. I'm not sure if this change makes things clearer for the reader.

@encukou: thoughts?

@encukouencukouAug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be technically correct, it's not just types – PyModuleDef is also a PyObject* (though I assume that's for historical reasons only). But that's too much detail here.

@KrisThielemansKrisThielemans mentioned this pull request Apr 18, 2022
Closed
encukou
encukou reviewed Aug 4, 2022
View reviewed changes
Comment threadDoc/c-api/intro.rst
Comment on lines +239 to +240
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.

@encukouencukouAug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would this work?

Suggested change
declared. The sole exception are the type objects; they may be static
:c:type:`PyTypeObject` objects.
declared. (There are a few exceptions, like static type objects, but even those
are usually manipulated via pointers.)

@kylotankylotanmannequin mentioned this pull request Sep 19, 2022
Closed
iritkatriel
iritkatriel requested changes Dec 6, 2022
View reviewed changes

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has merge conflicts now.

@bedevere-bot

bedevere-bot commented Dec 6, 2022

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.

@koubaakoubaamannequin mentioned this pull request Jan 12, 2024
Open
@github-actions

github-actionsBot commented Apr 8, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Apr 8, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@encukouencukouencukou left review comments
@shihai1991shihai1991shihai1991 left review comments
@erlend-aaslanderlend-aaslanderlend-aasland left review comments
@iritkatrieliritkatrieliritkatriel requested changes
+1 more reviewer
@eamanueamanueamanu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting changesdocsDocumentation in the Doc dirskip newsstaleStale PR or inactive for long period of time.

Projects

Docs PRs
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@koubaa@vstinner@bedevere-bot@encukou@iritkatriel@eamanu@shihai1991@erlend-aasland@the-knights-who-say-ni@ezio-melotti