-
Notifications
You must be signed in to change notification settings - Fork 826
Clean up Python
documentation
#1963
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
Conversation
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.
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. |
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.
Is it worth expanding on GIL for this introduction? Possibly also with a link to some Python docs.
/// 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 |
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.
"owner by the Python interpreter" isn't quite true - the references are owned by PyO3 (but point to Python objects).
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.
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".
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.
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?
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
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.
Looks great to me, thanks again!
Adds/changes some documentation in the python.rs module.