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

Remove redundant Python 3.6 code #9461

Merged
merged 1 commit into from Dec 30, 2021
Merged

Remove redundant Python 3.6 code #9461

merged 1 commit into from Dec 30, 2021

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Dec 30, 2021

Follow on from #9442 / #9437, remove some more redundant code for EOL Python 3.6, part two.

Sorry for the second PR, I missed some stuff in #9460: because feature branches aren't built on the CI, I usually temporarily remove the restriction, but this time did it in main and sent the PR from the old feature branch instead.

Relatedly, perhaps remove the feature branch restriction?

@nicoddemus
Copy link
Member

nicoddemus commented Dec 30, 2021

Sorry for the second PR

No worries!

because feature branches aren't built on the CI

Hmm what you mean? Our configuration currently builds master and the maintenance branches:

on:
push:
branches:
- main
- "[0-9]+.[0-9]+.x"

What do you mean by "feature branches"?

@nicoddemus nicoddemus merged commit 3c8c0d2 into pytest-dev:main Dec 30, 2021
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@RonnyPfannschmidt
Copy link
Member

@nicoddemus got 🥇

@hugovk
Copy link
Member Author

hugovk commented Dec 30, 2021

because feature branches aren't built on the CI

Hmm what you mean? Our configuration currently builds master and the maintenance branches:

on:
push:
branches:
- main
- "[0-9]+.[0-9]+.x"

What do you mean by "feature branches"?

I meant they're not built on my fork's CI.

An example:

Following the contribution guide, I checkout a new feature branch called your-bugfix-branch-name from main:

https://github.com/pytest-dev/pytest/blob/main/CONTRIBUTING.rst#long-version

$ git checkout -b your-bugfix-branch-name main
$ git commit -a -m "dummy commit, will the CI build this"
$ git push -u

Then, I can of course test locally, and usually do, but we have a powerful CI with a large matrix to test much more than I can (for example, lots of Python versions on multiple operating systems). It's usually quicker too, and has a nice clean env each time.

And to avoid noise I want to avoid opening a PR which then immediately fails the CI (I'm using a Mac and maybe missed something for Windows), so I check GitHub Actions on my own fork to address any failures first:

https://github.com/hugovk/pytest/actions

But oh, there's no build for the "dummy commit, will the CI build this" commit (hugovk@3c2c3f7).

Options:

  • Just open a PR, cross fingers it passes, if not, iterate, and ping the dozens of watchers each time
  • Make my changes on my main branch instead, not ideal as I only have one main; my main should be like upstream main and feature branches are the way to use Git
  • Temporarily edit the config to enable feature branches for the CI (e.g. hugovk@c195c63 -> https://github.com/hugovk/pytest/actions/runs/1637790911 builds), iterate until ready to create PR, rebase out the temp commit, then create the PR

It would 's a nicer contribution path if feature branches are buildable on forks without any extra steps, and before opening PRs.

@bluetech
Copy link
Member

@hugovk Will creating a PR first against your own fork's main work?

@hugovk
Copy link
Member Author

hugovk commented Dec 30, 2021

Yes, it will: hugovk#6

Although I accidentally created it against upstream because GitHub defaults PRs there: #9463 (sorry!)

@nicoddemus
Copy link
Member

Ahh got it, thanks.

Yeah not sure why we have that restriction in place TBH. I'm OK with removing and just building all branches.

Just open a PR, cross fingers it passes, if not, iterate, and ping the dozens of watchers each time

Having said the above, I think it is fine to open a Draft PR for that purpose: it conveys well that people are not supposed to be reviewing it just yet.

@hugovk
Copy link
Member Author

hugovk commented Jan 1, 2022

PR created: #9467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants