Skip to content

Backport of PR 12686 on v2.2.x #12698

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Backport of PR 12686 on v2.2.x #12698

wants to merge 1 commit into from

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Nov 1, 2018

PR Summary

Backports of #12686 and #12431.

Note: I only backport the code of #12431, not the tests and test images. But I assume that's ok as changes are made against master and tested there.

@tacaswell tacaswell added this to the v2.2.4 milestone Nov 4, 2018
@tacaswell
Copy link
Member

Master has moved far enough away from the 2.2.x branch is is probably better to backport the tests as well.

tacaswell
tacaswell previously approved these changes Nov 4, 2018
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I am 50/50 on backporting the tests. Defer to a second reviewer.

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Nov 8, 2018 •
edited
Loading

This may be a bit naive, but I'd say one should always backport the tests. Even with tests it's still possible to break things unconsciously, but at least it ensures not to break the very feature that is added.
With fewer and fewer users actively using the 2.2 branch it's also more likely that if something breaks it will not immediately be discovered.

If this gets backported to 2.2 then #12673 should as well, right?

@tacaswell
Copy link
Member

Oh, good catch @ImportanceOfBeingErnest !

I miss-read things, I am -0.5 on backporting these PRs as they are not critical bugs in 2.2.x

@tacaswell tacaswell dismissed their stale review November 11, 2018 03:56

Misread the PR, thought this was fixing an existing issue with the collections.abc imports (which will eventually become an import failure).

@timhoffm
Copy link
Member Author

Fine with not backporting.

@timhoffm timhoffm closed this Nov 12, 2018
@timhoffm timhoffm deleted the backport-of-pr-12686-on-v2.2.x branch November 12, 2018 21:57
@QuLogic QuLogic modified the milestones: v2.2.4, unassigned Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timhoffm @tacaswell @ImportanceOfBeingErnest @QuLogic