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

bpo-44249 Update README.rst #26385

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

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

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

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

This comment has been minimized.

@zware

zware May 27, 2021
Member

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.

This comment has been minimized.

@terryjreedy

terryjreedy May 28, 2021
Member

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

This comment has been minimized.

@zware

zware May 27, 2021
Member

I don't think this needs to change.

This comment has been minimized.

@terryjreedy

terryjreedy May 28, 2021
Member

I was 50/50, so reverted.

README.rst Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
updated and corrected readme with some grammatical mistakes

This comment has been minimized.

@zware

zware May 27, 2021
Member

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

This comment has been minimized.

@terryjreedy

terryjreedy May 28, 2021
Member

I removed it.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 27, 2021

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

@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 <zachary.ware@gmail.com>
Copy link
Contributor Author

@Ayushparikh-code Ayushparikh-code left a comment

Done

@Ayushparikh-code
Copy link
Contributor Author

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

I have made the requested changes; please review again

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 28, 2021

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
Copy link
Contributor Author

@Ayushparikh-code Ayushparikh-code left a comment

(●'◡'●)

@terryjreedy terryjreedy self-assigned this May 28, 2021
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
Comment on lines +113 to +114

This comment has been minimized.

@terryjreedy

terryjreedy May 28, 2021
Member

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

This comment has been minimized.

@terryjreedy

terryjreedy May 28, 2021
Member

I am reverting except for 's.

@terryjreedy
Copy link
Member

@terryjreedy terryjreedy commented May 28, 2021

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

@zware
zware approved these changes May 28, 2021
@terryjreedy terryjreedy merged commit acac6c7 into python:main May 28, 2021
11 checks passed
11 checks passed
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
Azure Pipelines PR #20210528.36 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 44249 found
Details
@bedevere-bot
bedevere/news "skip news" label found
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 28, 2021

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

@bedevere-bot bedevere-bot commented May 28, 2021

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

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

Co-authored-by: Ayush Parikh <ayushparikh332@gmail.com>
miss-islington added a commit to miss-islington/cpython that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)

Co-authored-by: Ayush Parikh <ayushparikh332@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 28, 2021

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 <ayushparikh332@gmail.com>
terryjreedy pushed a commit that referenced this pull request May 28, 2021
(cherry picked from commit acac6c7)

Co-authored-by: Ayush Parikh <ayushparikh332@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment