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

Clean up Python documentation #1963

Merged
merged 5 commits into from Nov 2, 2021
Merged

Clean up Python documentation #1963

merged 5 commits into from Nov 2, 2021

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Nov 1, 2021

Adds/changes some documentation in the python.rs module.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Great clean-ups, thank you! Some quick thoughts from reading below...

@@ -99,26 +99,42 @@ impl PartialOrd<(u8, u8, u8)> for PythonVersionInfo<'_> {
}
}

/// Marker type that indicates that the GIL is currently held.
/// A marker token that represents holding the GIL.
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth expanding on GIL for this introduction? Possibly also with a link to some Python docs.

Suggested change
/// A marker token that represents holding the GIL.
/// A marker token that represents holding the Python interpreter's Global Interpreter Lock (GIL).

/// [`Python::new_pool()`]), which can cause surprising results with respect to
/// when a variable's reference count is decreased so that it can be released to
/// the Python garbage collector. For example:
/// The [`Python`] type can be used to create references to variables owned by the Python
Copy link
Member

Choose a reason for hiding this comment

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

"owner by the Python interpreter" isn't quite true - the references are owned by PyO3 (but point to Python objects).

Copy link
Member Author

Choose a reason for hiding this comment

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

I can see how the wording is confusing.

I'm (in general) not happy with how "references" are explained so far. Maybe it would be better to just use "owned (or strong) reference" and "borrowed (or weak) reference" everywhere, rather than "GIL-independent" and "GIL-dependent" or just "reference".

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Perhaps over the holiday season I will finally get another chance to look at #1056 and we can come up with better terminology then?

src/python.rs Outdated Show resolved Hide resolved
src/python.rs Show resolved Hide resolved
mejrs and others added 2 commits November 1, 2021 20:02
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks again!

@davidhewitt davidhewitt merged commit 7b9ae8e into PyO3:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants