-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PEP 594: add myself as co-author and target for Python 3.11 #2262
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.
Thanks, a couple of pieces of feedback
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.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.
Thanks for helping to push this forward!
Does it sound like the SC would accept this? That would be great! |
Excited to see this finally moving forward again! Happy to assist in seeing this through; if I can help with PEP updates/editing or the more mechanical tasks in the CPython repo, feel free to ping me here or on the Discourse thread. Review in progress... |
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.
Thanks again, @brettcannon ! I've got a mix of small textual suggestions and substantive content questions/comments. The one thing I didn't check was maintainer/expert status of the modules, as that would be rather time-intensive, but I can if that would be helpful. Thanks!
Also, @brettcannon , don't forget to add yourself to |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@JelleZijlstra I unfortunately wasn't done with this PR (I'm actually actively editing it right now and pushing my branch as I go), hence the unresolved conversations. I'll create a separate PR for the changes I'm currently making. |
Sorry for that! I interpreted the review request as an indication that you were ready. |
I actually didn't make the review request; GitHub did automatically when I edited |
Just FYI @brettcannon @JelleZijlstra using Github's "Draft" option for creating the PR (or "convert to draft" under the reviewers list afterwards) is very useful for situations like these, to prevent others merging it while you're working on it and send a clear signal that it is a work in progress. |
@CAM-Gerlach correct, but the PR was submitted in a form I was ready to merge. You just happened to disagree. 😉 So this wasn't an issue about me putting up a PR I wasn't to have merged from the start, but Jelle seeing a buggy signal from GitHub that somehow I was ready for another round of reviews and thus was done. I don't think constantly flipping a PR from draft to ready while addressing reviews is a common practice. |
Oh sorry, I wasn't referring to my review comments at all—what I meant was what what I presumed were other additional changes you were continuing to iterate on yourself (the
But maybe I misunderstood what you were saying there? I certainly wasn't suggesting flipping draft mode back and forth when responding to discussions, just while you still had changes you wanted to make on the PR (or there was some other important reason why it shouldn't be merged once approved). For avoiding merges when there are unresolved discussions, we could consider enabled the corresponding feature of GitHub branch protection which puts an alert and a count if there are unresolved review comments in the merge box, and ensures discussions are resolved (or the check is explicitly overriden) before merging. It really has come in handy on our various repos, since it can get hard to keep track (even by those involved) if there are a lot of discussions or a lag between them. |
I was referring to Jelle merging the PR while I was in the middle of addressing your review comments. Other stuff I am doing for the PEP are going to be done in separate PRs. |
Oh okay, gotcha. Sorry I misunderstood and thanks for clarifying! |
No description provided.