Nice find! Though I think it is better just to fail, so that someone realises it is a mistake. At least, I can just imagine writing code but forgetting to return the object, and trying and trying and never getting it to work -- would be so frustrating! And since presently it does fail too, it would not introduce regressions.

Also, once we have decided on what the path should be, a test case and an entry in the 1.12.1 release notes..... Thanks!

Though I think it is better just to fail, so that someone realises it is a mistake.

Yeah, i think I agree with you, and just did it this way because that seemed like what the original author was aiming for.

ValueError("__array_wrap__ did not return a value") seem reasonable as a message?

also while we're at it, can we put a deprecation warning in the missing context argument codepath?

Actually, can't we just allow it to return None? Should be pretty obvious it's a mistake if arr.view(Foo) + 1 returns None, but there's a small chance someone might actually want that behaviour?

Since it is possible to return anything at all, I think I agree None should not really be an exception to that rule. So then the solution is to just remove the whole if-statement, right? That has the nice side-benefit of making the normal path just a little bit faster.

p.s. As for deprecating the no-context part: definitely a different PR (since it doesn't fix any bugs, so would go in 1.13, not 1.12.1). Not completely sure it is worth it, though -- given the comparison with NULL, this is a fast path for the regular case and I'm not sure it is worth the churn (especially since we should have our new & shiny __array_func__ in the not-too-distant future...).

So then the solution is to just remove the whole if-statement, right?

Yep, done, along with a testcase

definitely a different PR

Agreed

Not completely sure it is worth it, though

My concern is other bits of numpy that forget to do this check.

and an entry in the 1.12.1 release notes

Is there a template I should be creating these from?

@charris - this PR is a bug fix for 1.12, and should presumably be mentioned in the release notes, but there is no 1.12.1-notes.rst file yet; should one just use, e.g., 1.11.3-notes.rst as a template?

@mhvk The merged fixes will be found by a script and added to the 1.12.1 release notes with a link to the relevant PR. Unless you think more explanation is needed that should suffice. I've created the initial release notes just in case, but you will need to rebase this to see them.

OK, thanks for the clarification. In that case, @eric-wieser, I think this is ready to go in after you've made the two small changes I requested in-line (you can use [skip ci], since this will not affect the tests).

(you can use [skip ci], since this will not affect the tests).

Presumably then, I should not squash my commits?

@eric-wieser - arrgghh - my astropy background, where we do not worry about the number of commits, is showing through. Since for numpy the changelog is partially generated automatically, based on commits, I guess it is indeed much bettter to just have a single commit (unless there really are different pieces to a puzzle). So, could you squash?

@mhvk I'm thinking of putting the PRs in the changlog instead of commits/merges. Note that the last posted release notes contained PRs with a link. These days you can also do squash and merge and our script also picks that up. Just make sure that the summary line and commit message are appropriate and the summary line ends with (pr number) as it currently does by default.

Ok, squashed

Great, merging...

This needs a rebase on 1.12.1, right?

I think the backporting gets done in separate PRs -- @charris??

Yep, it definitely goes in a separate PR. Not sure if it's automated, in which case this needs to stay on the radar

something I like to do with bugfixes intended for backporting from the beginning is to rebase them onto the merge base between the maintenance branch and the master branch.
That way the same commit can be merged into both branches which saves you pushing a new branch (just create another PR from the same branch) and allows using git tag --contains command to see which tags/branches contain a bugfix

on the bugfix branch:

git rebase --onto $(git merge-base master maintenance/1.12.x) HEAD^

I usually do it, but others not so it is optional.

Yep, that would have been a good idea. Too late now though...