Revisit PR template [ci skip] by blueyed · Pull Request #6104 · pytest-dev/pytest

blueyed · 2019-10-29T19:04:54Z

make it clear that the text should be deleted, which is often not the
case - resulting in handling it as incomplete TODOs
clarify targetting master/features and make it a single actionable
item. Ref: Replace py.io.TextIO with io.StringIO #6064 (comment)
fix punctuation

- make it clear that the text should be deleted, which is often not the case - resulting in handling it as incomplete TODOs - clarify targetting master/features and make it a single actionable item. Ref: pytest-dev#6064 (comment) - fix punctuation

asottile · 2019-10-29T19:15:24Z

.github/PULL_REQUEST_TEMPLATE.md

+!! Please delete this text from the final description, this is just a guideline) !!
+
+- [ ] Target the `master` branch for bug fixes, documentation updates and
+      trivial changes.  But please use `features` for changes touching many


isn't the opposite slightly better (since we frequently merge master into features but rarely the other way)

The point here is that it should go to the leading branch, and not the stable/release branch (for bugfixes).

@asottile
Can't you see / understand that?

bluetech · 2019-10-29T20:03:09Z

May I ask how the master/features strategy came about? I am sure there is a reason for it, but I'm curious about the rationale. Superficially it seems complicated, requiring contributors to understand the difference and target the right branch, and requires frequent cross-merging and "syncing".

How about scheme like the following?

There is no features branch.
All regular PRs (features, bug fixes, trivial changes) go to master.
For each feature release (5.3.0), create a branch off of master (5.3.x), and tag the release there (v5.3.0) .
For patch releases, a maintainer cherry-picks into the release branch (5.3.x) from master, and tags the release there (v5.3.1).

This eliminates the frequent merging and the master/features split, in exchange for a little bit more work for the maintainer (can use a bot for this!), and that the tagged commits and version bumps are not found directly in master.

This is similar to what Django uses for example, described here.

nicoddemus · 2019-10-29T20:14:23Z

Hi @bluetech,

Not sure, it seems the approach you propose would mean more work on the maintainers side, wouldn't it? I mean, in practice, it might mean we would make more feature releases than patch releases, because of the work involved.

blueyed · 2019-10-30T07:42:22Z

I agree with @bluetech here - this is a more common workflow in general, where master is just for development basically.
As for me I am treating features as (common) master in this regard basically, and master as what we would have with a release branch.

@nicoddemus

Not sure, it seems the approach you propose would mean more work on the maintainers side, wouldn't it?

It's basically what we have with 4.6-maintenance already.

I mean, in practive, it might mean we would make more feature releases than patch releases, because of the work involved.

Wouldn't really be bad, is it?
We could just mark PRs for backporting, and merge / cherry-pick all of them before a bugfix release then (if there are enough, and or if there is a regression / fix required after a feature release).

Keep in mind that we currently have to merge master into features all the time already (to get fixes into "features" etc).

RonnyPfannschmidt · 2019-10-30T08:55:41Z

if we where using a scm that uses sound patch theory, say darcs or pijul, then i wouldnt mind such a workflow, as patch exchange would be much more sane.

however git makes things a pain, so i'd rather avoid that

id rather like to see a flow without feature branches and all releases coming off master often and early

The-Compiler · 2019-10-30T09:08:36Z

FWIW what @bluetech describes in #6104 (comment) is what I did in qutebrowser for the past 5 years. It works quite well for me - I have a feature release around all 2 months, and typically something like 2 patch releases after each.

For qutebrowser, I do all the cherry-picking myself for any changes where I feel like it's appropriate to do so. It's definitely some work though. Indeed it could be automated, but someone needs to do so first 😉

@RonnyPfannschmidt Not sure what you mean. Do you have some more details other than "git makes things a pain, other things are more sane" which doesn't really say anything?

blueyed · 2019-10-30T09:21:20Z

@The-Compiler
@RonnyPfannschmidt is referring to Git rewriting the object(s) necessarily (when cherry-picking).
To avoid that one method would be to use merge-bases: i.e. bugfix PRs should be based on the merge base (commit) from the last release: this then allows to merge them (as the same commit) into both release-X and master.
This requires some preparation then though - but could be done by maintainers easily before merging something then (by force-pushing a rebased version if it is decided that it should also go into a bugfix release).
But I do not think having different commit hashes on the release branch is bad, and that git cherry-pick -x is a good method to keep the referene to the original PR.

blueyed · 2019-10-30T09:24:20Z

Anyway, while we can continue the discussion here I think the change itself might make sense already for now, no?

The-Compiler · 2019-10-30T09:43:57Z

I agree, I don't see a problem with having two different commits for bugfixes.

I do see a problem with bugfix PRs being based on merge bases - I don't think this will work out, and it requires a lot of additional work for both maintainers and contributors.

blueyed · 2019-10-30T10:35:05Z

I do see a problem with bugfix PRs being based on merge bases - I don't think this will work out, and it requires a lot of additional work for both maintainers and contributors.

Yeah, it is a bit unnecessary, but could be used if that's the only reason @RonnyPfannschmidt does not approve this workflow.
It shouldn't be hard to have this scripted, using (from the PR's HEAD): git rebase -i $(git merge-base HEAD release-X).

asottile · 2019-10-30T15:32:52Z

if we where using a scm that uses sound patch theory, say darcs or pijul, then i wouldnt mind such a workflow, as patch exchange would be much more sane.

however git makes things a pain, so i'd rather avoid that

id rather like to see a flow without feature branches and all releases coming off master often and early

no separate branches and no cherry picks (except backports) is definitely the simplest workflow -- if there's features make a feature release, if there's bugfixes make a bugfix release (this is, afaict how hypothesis does things -- perhaps @Zac-HD can weigh in)

nicoddemus · 2019-10-30T18:11:02Z

no separate branches and no cherry picks (except backports) is definitely the simplest workflow

Personally I don't find the features/master workflow that complicated TBH, but if we are to change things, I would vote for this workflow then, although I think in practice this will mean most likely every release will be a minor release, given that we add improvements all the time (new features not so much).

blueyed · 2019-10-31T00:29:48Z

Personally I don't find the features/master workflow that complicated TBH

Me neither.
It fits the common / described idiom of "master" (development, maybe unstable - we call it "features") and "release-X" (bugfixes, we call it "master").
It's just that you would not change 40+ lines to clean up imports on release-X, but master instead.
But one downside of this is clearly, that while you might be using "features" all the time, you are missing bugfixes (until master gets merged) - normally you would merge/cherry-pick fixes from master to release-X.

Therefore it makes sense to me to change the flow to something more common, i.e. master + release branches. The latter would then also not be necessary until a hotfix is required really, and releases could just be made from master instead. If it is a major, minor, or patch release can be decided then.

Anyway, this PR seems to not get taken as-is anyway, closing.

blueyed · 2019-11-09T21:50:22Z

.github/PULL_REQUEST_TEMPLATE.md


 Here is a quick checklist that should be present in PRs.
-(please delete this text from the final description, this is just a guideline)
-->


As per #6159 (comment) removing the comments might be good already.
Happy to create a PR, but please approve it here already, so that it does not get talked down again.

I think the "(please delete...)" could be removed from here, but I'd leave the other instructions inside comments, otherwise you'll see them on many PRs.

hugovk · 2019-11-10T06:25:31Z

.github/PULL_REQUEST_TEMPLATE.md


 - [ ] Create a new changelog file in the `changelog` folder, with a name like `<ISSUE NUMBER>.<TYPE>.rst`. See [changelog/README.rst](https://github.com/pytest-dev/pytest/blob/master/changelog/README.rst) for details.
- [ ] Add yourself to `AUTHORS` in alphabetical order;
+- [ ] Add yourself to `AUTHORS` in alphabetical order


Big +1 for removing this semicolon!

It's not necessary here, you don't use them at the end of a sentence; they would only go in the middle of a sentence, a bit like a comma.

@hugovk
Please consider creating a PR with that and your other suggestion.

Please see PR #6188.

Revisit PR template [ci skip]

dc02cf4

- make it clear that the text should be deleted, which is often not the case - resulting in handling it as incomplete TODOs - clarify targetting master/features and make it a single actionable item. Ref: pytest-dev#6064 (comment) - fix punctuation

blueyed mentioned this pull request Oct 29, 2019

Replace py.io.TextIO with io.StringIO #6064

Merged

asottile reviewed Oct 29, 2019

View reviewed changes

blueyed closed this Oct 31, 2019

blueyed deleted the pr-template branch October 31, 2019 00:29

blueyed mentioned this pull request Nov 1, 2019

Merge master into features #6111

Merged

This was referenced Nov 9, 2019

add a ci bot that checks for news fragment existence or a trivial marker #6080

Open

[RFC] assert: keep "where … and …" output in verbose mode #5933

Closed

blueyed commented Nov 9, 2019

View reviewed changes

hugovk reviewed Nov 10, 2019

View reviewed changes

Uh oh!

Conversation

blueyed commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluetech commented Oct 29, 2019

Uh oh!

nicoddemus commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueyed commented Oct 30, 2019

Uh oh!

RonnyPfannschmidt commented Oct 30, 2019

Uh oh!

The-Compiler commented Oct 30, 2019

Uh oh!

blueyed commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueyed commented Oct 30, 2019

Uh oh!

The-Compiler commented Oct 30, 2019

Uh oh!

blueyed commented Oct 30, 2019

Uh oh!

asottile commented Oct 30, 2019

Uh oh!

nicoddemus commented Oct 30, 2019

Uh oh!

blueyed commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nicoddemus commented Oct 29, 2019 •

edited

Loading

blueyed commented Oct 30, 2019 •

edited

Loading