It might be worth adding type checks for some of the other arguments and value checks (e.g. that precision >= 0), if you feel up to it, but I agree that this solves the error mode in #7852, which is probably the most important to deal with.

Yeh. I'll leave it for now, as I think the main issue is passing a dict or some other object as the only argument, and in general we should be promoting duck typing

Well, there are two somewhat conflicting principles here: Duck typing is a good thing, but failing early rather than late is also a good thing :-). If we can tell at set_printoptions time that we're just going to fail later, then it's better to raise the error immediately. But I also agree with @rkern that this is an improvement.

Code contents

Sorry for the misclick...

Code comments:

• There should be a comment noting why this particular argument needs to be checked (because it's the first positional argument and we want to catch this common mistake where it's a dict)
• Like all changes, this needs a test added to make sure it works and keeps working.

Thanks for contributing!

No worries!

I originally included a comment explaining why the error was being raised, but it was suggested I remove it. I will add it back in if @rkern is happy?

Ah ok, I'm new to this - could you possibly run me through the process of adding the test?

I would argue that duck typing is easy, just do precision = operator.index(precision), or something similar.
You can find tests in numpy/core/tests/ check for the right file (probably test_arrayprint.py and find some place that seems good. The tests are pretty self explanatory.

EDIT: of course operator.index is assuming that an ssize_t is sufficient for the precision keyword ;).

I dislike the comment. The only reason it might be desirable is to explain why there is only type-checking on the one argument. I'd rather you just add basic type-checking to the other arguments instead.

reason it might be desirable is to explain why there is only type-checking on the one argument

Well, yes, exactly ;-)

For adding a test: tests are just functions whose name starts with test_, and that live in one of the test files, and that call assert_* functions to check things. There are probably some tests for printing already; if you grep for the string "printoptions" then you'll probably find the right place to add your test. Here you'll want to use assert_raises (again, grep will find you other examples in the codebase), and to run the tests just run ./runtests.py in the root of your checkout. (Plus tests will also be run automatically when you push new changes to the PR.)

OK. Well it's 1 v 1 on the comment issue, but I decided to add it as I think it could be unclear to someone examining the code a to why there is only one type check.

I've added a test in the appropriate place, thanks for the explanation.

I've also combined my commits into one to clean it all up. Hopefully this all done and dusted now :)

If you are going to make a comment, make it a comment and not part of the docs. It's only a puzzle to people reading the source code, not users.

But I'd really rather you add basic checks to the other arguments.

Well, as long as the error makes sense in the other cases, it would not matter, but otherwise I think Robert is right, there is not much gain by not adding the other checks to have a more reasonable error.

As to the isinstance check. In cetero censeo: Use operator.index instead to convert it in a duck-typing way. I seriously dislike that isinstance check, it will even fail some (on python 3 all) numpy integers.

Good points, I will add the operator.index logic instead - duck typing is better and I didn't realise isinstance could fail on numpy ints.

I will check all the int parameters that are passed, is there an equally clean way to check the bool or string parameters? I.e. using duck typing.

Yes, when I said comment I meant a comment next to the check, not anything in the docstring.

For type checks: Probably just calling bool(...) and str(...)? I guess that's effectively what will happen later anyway, either explicitly or implicitly...

I left the boolean argument - it was already parsed by not not, but I've added parsing the string arguments with str(...). All the integer arguments are now parsed by operator.index(...), and I removed the comment.

I think now we're implementing duck typing in the right way, raising errors when we need to.

Just to note, that I realized that operator.index is slightly more strict then the current behaviour (by not allowing things such as floats). Not sure we should be bothered by it or not.

However, since it is a slight change in behaviour, if we do it, we still might want to note it in the release notes for completeness.

☔ The latest upstream changes (presumably #8983) made this pull request unmergeable. Please resolve the merge conflicts.

@tom-bird this seems like a nice addition, feel like rebasing it (to fix merge conflicts) and adding a release note?

At this point, this PR will probably need to restart from scratch - none of the modified lines exist any more. The idea is still good though, so if we close this PR we should open an issue to replace it.

Going to close this, the original issue remains open. @tom-bird If you want to give it another shot please open a new PR. @BvB93 Do the annotations help with this?

@charris I would imagine so, yes.
Though at first the glance i suspect that somewhat convoluted nature of np.core.arrayprint will make annotating it a bit challenging (needs further investigation).