Skip to content

bpo-44249 Update README.rst #26385

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

Merged
merged 10 commits into from
May 28, 2021
Merged

bpo-44249 Update README.rst #26385

merged 10 commits into from
May 28, 2021

Conversation

Ayushparikh-code
Copy link
Contributor

@Ayushparikh-code Ayushparikh-code commented May 26, 2021

Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

This is too trivial for a news entry IMHO.

I also, forget if we can really merge changes if authors have not signed the CLA.

@Ayushparikh-code Ayushparikh-code changed the title Update README.rst Update README.rst #44249 May 27, 2021
@Ayushparikh-code
Copy link
Contributor Author

This is too trivial for a news entry IMHO.

I also, forget if we can really merge changes if authors have not signed the CLA.

I signed CLA , look over it again

@Ayushparikh-code Ayushparikh-code changed the title Update README.rst #44249 bpo:44249 Update README.rst May 27, 2021
@Ayushparikh-code Ayushparikh-code changed the title bpo:44249 Update README.rst bpo-44249 Update README.rst May 27, 2021
zware
zware previously requested changes May 27, 2021
README.rst Outdated
workload. This is necessary in order to profile the interpreter execution.
Note also that any output, both stdout and stderr, that may appear at this step
workload. This is necessary to profile the interpreter's execution.
Note also that any output, both stdout, and stderr, that may appear at this step
Copy link
Member

Choose a reason for hiding this comment

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

This should not change; it's not a list, it's just a sub-section of the sentence. I'm not sure this note is true anymore, though.

Copy link
Member

Choose a reason for hiding this comment

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

I am reverting except for 's.

README.rst Outdated
is suppressed.

The final step is to build the actual interpreter, using the information
collected from the instrumented one. The end result will be a Python binary
collected from the instrumented one. The result will be a Python binary
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to change.

Copy link
Member

Choose a reason for hiding this comment

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

I was 50/50, so reverted.

@@ -0,0 +1 @@
updated and corrected readme with some grammatical mistakes
Copy link
Member

Choose a reason for hiding this comment

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

This entry is definitely not needed, but NEWS entries should also have proper grammar :)

Copy link
Member

Choose a reason for hiding this comment

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

I removed it.

@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.

Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

@zware, please do the honors of closing the PR unless @Ayushparikh-code has any rebuttals for the changes proposed.

Edit: I think I misread some of the reviews I guess to mean the change wasn't necessary, my bad.

Co-authored-by: Zachary Ware <[email protected]>
Copy link
Contributor Author

@Ayushparikh-code Ayushparikh-code left a comment

Choose a reason for hiding this comment

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

Done

@Ayushparikh-code
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from zware May 28, 2021 02:29
Copy link
Contributor Author

@Ayushparikh-code Ayushparikh-code left a comment

Choose a reason for hiding this comment

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

(●'◡'●)

@terryjreedy terryjreedy self-assigned this May 28, 2021
Comment on lines +113 to +114
flags for each flavor. Note that this is just an intermediary step. The
binary resulting from this step is not good for real-life workloads as it has
Copy link
Member

@terryjreedy terryjreedy May 28, 2021

Choose a reason for hiding this comment

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

These two changes and one other are correct. Will merge when CI done.

README.rst Outdated
workload. This is necessary in order to profile the interpreter execution.
Note also that any output, both stdout and stderr, that may appear at this step
workload. This is necessary to profile the interpreter's execution.
Note also that any output, both stdout, and stderr, that may appear at this step
Copy link
Member

Choose a reason for hiding this comment

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

I am reverting except for 's.

@terryjreedy
Copy link
Member

@nanjekyejoannah Trivial changes like this, with no creative content, do not need a CLA, But I still think getting it is better. If nothing else, so the matter is taken care of for next suggestion.

@terryjreedy terryjreedy merged commit acac6c7 into python:main May 28, 2021
@miss-islington
Copy link
Contributor

Thanks @Ayushparikh-code for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-26434 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)

Co-authored-by: Ayush Parikh <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 28, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)

Co-authored-by: Ayush Parikh <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 28, 2021
@bedevere-bot
Copy link

GH-26435 is a backport of this pull request to the 3.9 branch.

terryjreedy pushed a commit that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)


Co-authored-by: Ayush Parikh <[email protected]>
terryjreedy pushed a commit that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)

Co-authored-by: Ayush Parikh <[email protected]>
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants