Enforce boolean types
State: closed

- [ x ] closes #14189
- [ x ] tests added / passed
- [x] passes `git diff upstream/master | flake8 --diff`
- [x] whatsnew entry

This PR only addresses the `inplace` argument, though there was a comment on #14189 that identifies the issue as being with the `copy` argument as well. I can build this out further to account for all of the frequently occurring boolean arguments.

I also wrote tests for the common function `_enforce_bool_type`, but didn't write individual tests for every method with an `inplace` argument. This is something I can add if it makes sense. I wanted to get something reviewed sooner rather than later to get feedback and ensure that I'm on the right track. Feedback is much appreciated -- thanks!


Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
call this `_validate_bool_type`, should accept `None` as valid. (just ignore)

use `pandas.types.common.is_bool` (for consistency with other routines)

put this in `pandas.types.validate` (make a new module). add tests in `pandas.tests.types.test_validate`. Need to cover each routine that is covered (just a smoke test, IOW, just loop thru a couple of invalid values and make sure it raises)

thanks for the feedback @jreback, will work on these!

@rforgione Do you have time to update this?

@jorisvandenbossche yeah, apologies. Things have been a little crazy with my day job of late. I'll dig into this over the weekend. 

@rforgione No problem, just ping here when you updated it

hey @jorisvandenbossche / @jreback -- sorry for the delay on this. I think I knocked out more-or-less all of the feedback items listed by @jreback above. I struggled a little with some of the tests for internals.py, namely for the putmask method on the NonConsolidatableMixIn class, and the _replace_single method on the ObjectBlock class (I had trouble instantiating the appropriate classes to run tests on). I'd be happy to write tests for those if you wouldn't mind pointing me in the right direction. Any other thoughts/feedback are appreciated. Thanks!
@rforgione looks pretty good. just a few more comments.
pls add a whatsnew for 0.20.0.


One concern about this PR I have is that if you now call a method with argument with a wrong type (eg like `df.where(lambda x: x > 4, lambda x: x + 10, 'fail')` as the original example in the issue), you simply get the error message "ValueError: Expected type bool, received type str". From this it is not clear at all that it is about the inplace keyword. 
So I think you should either make this validation function specific for `inplace` and add that to the error message, or either pass in the keyword name we are checking to the `_validate_bool_type` function (to include it in the error message) in all places.
@jorisvandenbossche I like the second suggestion. I can add a second argument to the `_validate_bool_type` function and pass the argument name as a string in each instance throughout the code base (so if I'm testing the `inplace` argument, I can call `_validate_bool_type(inplace, 'inplace')` where `_validate_bool_type` will use the string to create a more specific error message). Is that what you had in mind @jorisvandenbossche ?
@jreback @jorisvandenbossche I pushed the discussed changes, feel free to take a look. Any additional feedback is appreciated!
## [Current coverage](https://codecov.io/gh/pandas-dev/pandas/pull/14318?src=pr) is 84.77% (diff: 100%)
> Merging [#14318](https://codecov.io/gh/pandas-dev/pandas/pull/14318?src=pr) into [master](https://codecov.io/gh/pandas-dev/pandas/branch/master?src=pr) will increase coverage by **0.01%**

```diff
@@             master     #14318   diff @@
==========================================
  Files           145        145          
  Lines         51151      51208    +57   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43355      43413    +58   
+ Misses         7796       7795     -1   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [db08da2...2478d8f](https://codecov.io/gh/pandas-dev/pandas/compare/db08da2dfdb1740fdb67af3c29ab5867cd2602f8...2478d8ff41bfe39097c01b0317a52f93b0fb2635?src=pr)
just a couple of more changes to pass the Travis tests, will work on these tonight
just some very minor comments.

ping on green.
can you rebase
@jreback @jorisvandenbossche it's not immediately clear to me why the first set of tests is failing. I looked at the output in Travis but not seeing anything (literally blank output). 

![job 24181 3 - pandas-dev pandas - travis ci 2017-01-05 09-46-35](https://cloud.githubusercontent.com/assets/5572997/21684530/08f6cad0-d32c-11e6-8f7a-29fde5b1358b.png)

Any idea what's going on? Was about to rebase but wanted to check in on this first. 
@jreback @jorisvandenbossche figured it out -- I needed to re-build after I rebased against the upstream changes. All checks are on green, I think we are good to go!
@rforgione Thanks!