I submitted PR#138 on bpo-22807. I got a nice review from a community member
and made a small change. All checks have passed.
But now I'm stuck and I'm impatient. ;)
The change is small enough and I'm happy with it, and I could patiently wait
for another core dev to approve it, but in the likely case that no one else is
interested in the bug, I'd like to be able to self-approve and accept it.
This of course is described as my right in the devguide, along with the
responsibility to watch the buildbots and revert the change if there are any
problems with it.
But it seems like I cannot do that through the GH UI. Right now I'm seeing
(X) Review required
(✓) All checks have passed
(X) Merge is blocked
There seems to be no way to self-approve the PR. If I go to 'Files changed'
tab and click on 'Review changes', both Approve and Request changes is
dimmed. I can only comment on my own PR.
I can understand why we might want to prohibit self-approvals, but that's also
a regression in the permissions and rights of core developers. I also think
it's counterproductive since without that I might have to go begging for a
review approval before the branch can land.
Is this intentional or an oversight?
(As an owner of the project, I took a quick look at the project settings and
couldn't find a control for this, but I know other GH projects I contribute to
I didn't turn on Codecov requirements for the patch because we seem to
still have variance in them where some module that are inconsistently being
tested (there is a test for the whole project but I left that off as well).
I think we should definitely work towards fixing the coverage variance as I
would like to require no drop in code coverage at some point, but as of
right now it's wonky enough to be too much of a showstopper to flip on. I
have opened https://github.com/python/core-workflow/issues/38 to track this
and any modules whose coverage is inconsistent so we can eventually fix
this and require coverage doesn't decrease.
One thing that seems to be lacking: names of commenters.
In the recent thread about [Provide guidance on editing PRs prior to merge (#129)] there are several entries, from at least two people, and I have no idea who they are or exactly which one said what because their names are not showing up anywhere that I can see.
Granted I could go and look on GitHub directly, but this feels like a regression from our previous work flow.
(I'm not subscribed to this list, so please keep me on CC)
Was there any discussion about allowing core developers to bypass any of
the PR checks? Or was there an assumption that we'd just push directly
to the main repo?
For example, most of my contributions are to do with the Windows
installer. Due to the nature of installers, virtually nobody else is
confident to review anything complex, or even simple ones (such as
https://github.com/python/cpython/pull/160) due to not being able to
predict the flow on effects. As a result, I make many small changes that
would take a long time to get reviewed.
Similarly, many of my changes are not touched by the automated builds.
Particularly anything to do with the installer is certainly not going to
be built on a Linux box, and most (probably all) Windows buildbots don't
build the installer right now. I've seen some systems where tags can be
applied to a PR to skip build validation - I would certainly find that
But essentially, I believe it should be within the power of core
developers to bypass the checkin requirements (review + builds) and
force a merge. Currently this is not possible - could it be enabled?
Obviously it's placing trust in our core dev team, but that's the way it
has always been, and we don't lose any traceability from this (unlike
allowing people to delete branches from the repo, which can permanently
Hey cool, I've just submitted my first PR against GH, and it's *so* nice to be
able to use git, I might have to do this more often <wink>.
But I have questions about the coverage reports. In bbdef4c, coverage failed
because my diff wasn't 100% covered, and the module itself was missing some
coverage in code I didn't touch. If you click on the red X next to that
commit, you can see the codecov/patch report 94.73% of diff hit.
It's expected that some lines may be missed, due to underlying platform
support, so the natural thing would be to say so in the code. I.e. suppress
some misses so coverage doesn't false positive. This leads to questions:
* How is codecov.io actually performing coverage? It seems difficult to find
out exactly based on their documentation, and python/cpython's .codecov.yml
doesn't reveal any details. Over in #python-dev, nedbat guessed it would be
coverage.py (big surprise! :), and that would both make sense and be great,
since it's a well-known and well-loved tool.
* On that guess, I figured I'd add a few #pragmas for lines which are okay not
to be covered. I pushed the new commit and didn't see a codecov.io failure
so I thought that did the trick, but I'm actually not so sure. If you look
at the above PR, you can see that commit bbdef4c has a red X, clicking on
which gives you details that codecov/patch failed but travis-ci/pr
However, if you hover over the green check next to ccefb3a, all you see is
that the travis-ci/pr check succeeded. There's no indication that coverage
actually ran at all on ccefb3a. That doesn't make sense to me.
* How can you run the same coverage tests locally? It's hard to know, given
the apparent dearth of details from codecov.io. The top-level Makefile does
have a `coverage` target, but that's a different thing. It would be nice if
you didn't have to push a new commit just to see if you've fixed your
coverage issues. There needs to be a local way to run the same tests.
As I'm writing this, suddenly ccefb3a gained another check! So it seems like
codecov.io is quite delayed in reporting its coverage, and the PR results can
be misleading in the meantime, because it only reports that there is 1 check
until codecov.io completes. I would prefer to see a yellow dot (i.e. the test
is still running).
A massive thank you to everyone who helped with this. It has been a long
road but we finally reached the end of it!
As for what's next, I think getting Misc/NEWS straightened out and then
getting cherry-picking PRs automated are the next important items. As
always https://github.com/python/core-workflow/issues has the current ideas
on how to improve our workflow.
+1: Nick, Senthil, Chris
-0: Martin, Brett
-1: Naoki, Berker
(Maciej was positive but didn't say +1 or +0; Martin said -0.5 which isn't
a valid vote, so I rounded up for him; I'm personally on the fence so
voting conservatively now but can switch that view)
If you have an opinion please express them now so Senthil has a chance to
test this today before we do the official move tomorrow. Otherwise Senthil
and I will make the final decision ourselves.
Historically commit messages for CPython have had the form of "Issue #NNNN:
did something". The problem is that Github automatically links "#NNNN" to
GitHub issues (which includes pull requests). To prevent incorrect linking
we need to change how we reference issue numbers.
The current candidates are:
issue NNNN (notice the lack of #)
bpo NNNN ("bpo" stands for "bugs.python.org")
Whatever choice we go with it will be how we reference issues in PR titles
and comments to link the PR to the issue, and in commit messages to send a
message to the issue about the commit.
To start this off, I'm -1 on "issue" (because people will out of habit add
the #), +0 on "bug" (it's different but not everything is a bug), and +1 on
"bpo" (as it namespaces our issues).