Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Master has moved far enough away from the 2.2.x branch is is probably better to backport the tests as well. |
There was a problem hiding this 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 commentedNov 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. If this gets backported to 2.2 then#12673 should as well, right? |
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 |
Misread the PR, thought this was fixing an existing issue with the collections.abc imports (which will eventually become an import failure).
Fine with not backporting. |
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.