Skip to content

Conversation

@nanjekyejoannah
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah commented Apr 2, 2019

I have added documentation to clarify on what the main interpreter is.

https://bugs.python.org/issue36487

@nanjekyejoannah
Copy link
Contributor Author

cc @ericsnowcurrently , @ncoghlan

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

The flow of the text doesn't line up as well with this change. Consider splitting the original paragraph right before the final sentence, like this:

 you to do that.  You can switch between sub-interpreters using the	
+:c:func:`PyThreadState_Swap` function.
+
+<explanation of main interpreter>
+
+You can create and destroy sub-interpreters using the following
+functions:
 

I've also left a comment for the paragraph about the main interpreter.

Before sub-interpreters, there is a main interpreter which is the first one
created by the CPython runtime on startup when the ``python`` command is
run. The :c:func:`PyInterpreterState_Main` funtion returns a pointer to this
main interpreter's state.
Copy link
Member

Choose a reason for hiding this comment

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

This description of the main interpreter could be a bit more direct but the focus should remain within the context of subinterpreters. For instance, there's no need here to mention any specific API related to the main interpreter. Anyway, I've updated the BPO issue with a list of key points to cover.

FWIW, I'm not sure whether it would be better to keep the main-related text small (e.g. one short paragraph) or to make it comprehensive (e.g. across multiple paragraphs). If we go with that latter then it may make sense to have a separate section nearby for the main interpreter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to keep things minimal for now, and defer more comprehensive documentation to the resolution of the PEP for a Python level subinterpreter API.

Copy link
Contributor Author

@nanjekyejoannah nanjekyejoannah Apr 9, 2019

Choose a reason for hiding this comment

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

@ncoghlan I agree to this being a small paragraph for now most especially given the current structure of the docs. Though I also have to warn you that incorporating all the points @ericsnowcurrently suggests in the BPO will lead to a big paragraph.

@bedevere-bot
Copy link

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.

@nanjekyejoannah
Copy link
Contributor Author

nanjekyejoannah commented Apr 10, 2019

@ericsnowcurrently , I have made some edits.

Note: I just made a summary of your suggestions here : https://bugs.python.org/issue36487

it would have had much more detail if it was in a separate section though.

PTAL

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

That's looking good! Thanks for working on this. I just have a few comments.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

I finally figured out how the "Suggestions" UI works :)

(There's a hard-to-describe icon when editing your comments that adds the line you're commenting on with the markup to turn it into a suggested change)

nanjekyejoannah and others added 3 commits June 15, 2019 13:14
Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>
Co-Authored-By: Nick Coghlan <ncoghlan@gmail.com>
@nanjekyejoannah
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. This is looking good correctness-wise. I've left a few comments with tweaks to clarify a little and to make the paragraph flow a bit more cohesively. If you put them all together, the results looks like this:

The "main" interpreter is the first one created when the runtime initializes.
It is usually the only Python interpreter in a process.  Unlike sub-interpreters,
the main interpreter has unique process-global responsibilities like signal
handling.  It is also responsible for execution during runtime initialization and
is usually the active interpreter during runtime finalization.  The
:c:func:`PyInterpreterState_Main` funtion returns a pointer to its state.

@bedevere-bot
Copy link

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.

@nanjekyejoannah
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for working on this, @nanjekyejoannah!

@miss-islington
Copy link
Contributor

Thanks @nanjekyejoannah for the PR, and @ericsnowcurrently for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 2, 2019
…ythongh-12666)

(cherry picked from commit 854d0a4)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>
ericsnowcurrently pushed a commit that referenced this pull request Aug 2, 2019
…h-15080)

(cherry picked from commit 854d0a4) (gh-12666)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants