Skip to content

Conversation

@wcwagner
Copy link
Contributor

@wcwagner wcwagner commented Jul 17, 2016

@sinhrks
Copy link
Member

sinhrks commented Jul 17, 2016

Thx! can u add tests all methods uses str_pad, i.g. pad, center, ljust, rjust and zfill.

@sinhrks sinhrks added the Error Reporting Incorrect or improved errors from pandas label Jul 17, 2016
- Bug in ``groupby`` with ``as_index=False`` returns all NaN's when grouping on multiple columns including a categorical one (:issue:`13204`)

- Bug where ``pd.read_gbq()`` could throw ``ImportError: No module named discovery`` as a result of a naming conflict with another python package called apiclient (:issue:`13454`)
- Bug in ``pandas.Series.str.zfill``, no TypeErrors raised when ``width`` was not of integer type (:issue:`13598`)
Copy link
Member

Choose a reason for hiding this comment

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

nice to describe the results are all NaN. Also, TypeError

@sinhrks sinhrks added the Strings String extension data type and string data label Jul 17, 2016
@sinhrks sinhrks added this to the 0.19.0 milestone Jul 17, 2016
@wcwagner
Copy link
Contributor Author

I wrote all the tests, however it seemed extremely redundant doing so. I know that redundancy isn't necessarily a bad thing when testing, but is there a better way to implement them?

Thanks

"fillchar must be a character, not int"):
result = values.str.pad(5, fillchar=5)

def test_pad_width(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just put them all in a single test method and iterate over the functions asserting that they raise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback I'm relatively new to OOP in python, so could you let me know if there's anything I could do to improve my current implementation?
Thanks

@codecov-io
Copy link

codecov-io commented Jul 18, 2016

Current coverage is 84.38%

Merging #13690 into master will increase coverage by <.01%

@@             master     #13690   diff @@
==========================================
  Files           142        142          
  Lines         51223      51226     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43224      43227     +3   
  Misses         7999       7999          
  Partials          0          0          

Powered by Codecov. Last updated by ada6bf3...9669f3f

raise TypeError('fillchar must be a character, not str')

if not isinstance(width, compat.integer_types):
msg = 'width must be of integer type, not {0}'
Copy link
Contributor

Choose a reason for hiding this comment

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

use is_integer

@jreback jreback closed this in 5a52171 Jul 19, 2016
@jreback
Copy link
Contributor

jreback commented Jul 19, 2016

thanks @wcwagner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Error Reporting Incorrect or improved errors from pandas Strings String extension data type and string data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Series.str.zfill() doesn't check type

4 participants