You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We can also require the build pass before accepting a PR, although @dstufft suggests we don't require the PR to be up-to-date (which makes sense since most PRs will be contained to a single PEP).
Yea, two important things about requiring a build to pass before accepting a PR:
Requiring passing status checks means that all changes must go via PR, there's no more pushing directly to master.
If you require the PR to be up-to-date, it means every time a PR is merged, all other PRs need to be rebased or have master merged into them, which is a massive drag, so not requiring them to be up to date makes it easier to land changes with only a small window for breakage.
Oh, and specific to the first one, you can allow administrators to merge a PR even if things aren't passing (they get a "Are you sure?" prompt) as an escape hatch for the build being broken and needing multiple PRs to fix it.
Oh, and specific to the first one, you can allow administrators to merge a PR even if things aren't passing (they get a "Are you sure?" prompt) as an escape hatch for the build being broken and needing multiple PRs to fix it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
#17 is a first step, but needs to be further refined. For example, it should be ensured that markup warnings in RST PEPs should make the build fail, which I don't think the current Makefile allows.
Can somebody do something simple here? The discussion seems to have petered out with no clear conclusion. I'd say the better is the enemy of the good. I just want to see whether a PEP builds without warnings before merging it, since doing this manually is a huge pain.
@gvanrossum We're currently using Travis to run make on every PR and settings a failing status if that returns a non zero exit code. I think that means that warnings will get a passing check but failure to builds will not. You can see the output of running make by clicking on the details of the travis build (for example with #39 if you clicked on "Show all checks" then "details" next to Travis, you'd get https://travis-ci.org/python/peps/builds/140609559 and can scan that for warnings).
If we want to make it so that Travis will fail on warnings as well as on errors, then we'll need to expose a way in the Makefile / pep2html.py to ask it to turn warnings into errors.
I see, cool. It should be simple enough to make a change to pep2html.py to
do exit(1) on warnings, right? Or is there no way to get the warning status
from docutils/rest?
I'm not sure what the point is of making the test required. In the other
projects I'm familiar with (asyncio, mypy) we don't do this and I don't
think it has caused any problems.
It prevents accidental pushes to master since all changes then need to go through the PR process, this is particularly crummy when someone pushes something broken directly to master.
When you don't require status checks, Github doesn't show them until the status check has recorded some status ("pending", "success", "failed", or "error") but if you require them then it shows pending immediately. This means people don't have to go and look for which status checks passed to ensure that all of them have actually been reported but they can immediately see that there are still more status checks that haven't reported yet.
It helps ensure, that for whatever reason, master always stays passing.
Activity
brettcannon commentedon Jun 15, 2016
We can also require the build pass before accepting a PR, although @dstufft suggests we don't require the PR to be up-to-date (which makes sense since most PRs will be contained to a single PEP).
dstufft commentedon Jun 15, 2016
Yea, two important things about requiring a build to pass before accepting a PR:
master.mastermerged into them, which is a massive drag, so not requiring them to be up to date makes it easier to land changes with only a small window for breakage.dstufft commentedon Jun 15, 2016
Oh, and specific to the first one, you can allow administrators to merge a PR even if things aren't passing (they get a "Are you sure?" prompt) as an escape hatch for the build being broken and needing multiple PRs to fix it.
holdenweb commentedon Jun 17, 2016
As if :-)
Sent from my iPhone
birkenfeld commentedon Jun 20, 2016
#17 is a first step, but needs to be further refined. For example, it should be ensured that markup warnings in RST PEPs should make the build fail, which I don't think the current Makefile allows.
gvanrossum commentedon Jul 5, 2016
Can somebody do something simple here? The discussion seems to have petered out with no clear conclusion. I'd say the better is the enemy of the good. I just want to see whether a PEP builds without warnings before merging it, since doing this manually is a huge pain.
dstufft commentedon Jul 5, 2016
@gvanrossum We're currently using Travis to run
makeon every PR and settings a failing status if that returns a non zero exit code. I think that means that warnings will get a passing check but failure to builds will not. You can see the output of runningmakeby clicking on the details of the travis build (for example with #39 if you clicked on "Show all checks" then "details" next to Travis, you'd get https://travis-ci.org/python/peps/builds/140609559 and can scan that for warnings).If we want to make it so that Travis will fail on warnings as well as on errors, then we'll need to expose a way in the
Makefile/pep2html.pyto ask it to turn warnings into errors.dstufft commentedon Jul 5, 2016
#45 will ensure that even warnings fail the build and not just errors.
gvanrossum commentedon Jul 5, 2016
I see, cool. It should be simple enough to make a change to pep2html.py to
do exit(1) on warnings, right? Or is there no way to get the warning status
from docutils/rest?
I'm not sure what the point is of making the test required. In the other
projects I'm familiar with (asyncio, mypy) we don't do this and I don't
think it has caused any problems.
dstufft commentedon Jul 5, 2016
Making the tests required achieves a few things:
mastersince all changes then need to go through the PR process, this is particularly crummy when someone pushes something broken directly tomaster.masteralways stays passing.fgregg commentedon Aug 10, 2016
What is left on this issue now that #17 and #45 have been merged?
brettcannon commentedon Aug 10, 2016
Thanks for pointing out the staleness of this issue, @fgregg ! I added a Travis badge to close out this issue.
Merge pull request python#5 from willingc/pep-hamt-edit
Merge pull request python#5 from trishankatdatadog/trishankatdatadog/…
More examples (python#5)
Update control flow exception handling to acknowledge configurability…
Merge pull request #5 from LazyImportsCabal/ygh1s-updates
Merge pull request python#5 from savannahostrowski/frame-pointers