Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLN: remove __bytes__ #26447

Merged
merged 1 commit into from May 19, 2019

Conversation

@topper-123
Copy link
Contributor

commented May 18, 2019

Remove __bytes__ method from StringMixin and PandasExtensionDtype. These are the only uses of __bytes__ in the code base.

@topper-123 topper-123 force-pushed the topper-123:remove__bytes__ branch from 8dbaa07 to e1061be May 18, 2019

@codecov

This comment has been minimized.

Copy link

commented May 18, 2019

Codecov Report

Merging #26447 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26447      +/-   ##
==========================================
- Coverage   41.77%   41.69%   -0.09%     
==========================================
  Files         174      174              
  Lines       50754    50746       -8     
==========================================
- Hits        21205    21160      -45     
- Misses      29549    29586      +37
Flag Coverage Δ
#single 41.69% <ø> (-0.09%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/dtypes.py 73.91% <ø> (+0.46%) ⬆️
pandas/core/base.py 33.48% <ø> (+0.07%) ⬆️
pandas/io/gbq.py 15.78% <0%> (-57.9%) ⬇️
pandas/core/dtypes/concat.py 51.96% <0%> (-1.48%) ⬇️
pandas/io/formats/csvs.py 68.07% <0%> (-1.21%) ⬇️
pandas/io/formats/format.py 50.23% <0%> (-0.94%) ⬇️
pandas/core/internals/blocks.py 51.82% <0%> (-0.83%) ⬇️
pandas/core/dtypes/cast.py 48.59% <0%> (-0.34%) ⬇️
pandas/core/arrays/datetimelike.py 42.78% <0%> (-0.18%) ⬇️
pandas/core/arrays/datetimes.py 65.88% <0%> (-0.16%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80bddaf...e1061be. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented May 18, 2019

Codecov Report

Merging #26447 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26447      +/-   ##
==========================================
- Coverage   91.73%   91.73%   -0.01%     
==========================================
  Files         174      174              
  Lines       50754    50746       -8     
==========================================
- Hits        46560    46551       -9     
- Misses       4194     4195       +1
Flag Coverage Δ
#multiple 90.23% <ø> (ø) ⬆️
#single 41.69% <ø> (-0.09%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/dtypes.py 97.34% <ø> (+0.69%) ⬆️
pandas/core/base.py 97.97% <ø> (-0.02%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97.02% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80bddaf...2053dfa. Read the comment docs.

@@ -38,7 +38,6 @@ def test_tricky_container(self):
pytest.skip('Need unicode_container to test with this')
repr(self.unicode_container)
str(self.unicode_container)
bytes(self.unicode_container)

This comment has been minimized.

Copy link
@topper-123

topper-123 May 18, 2019

Author Contributor

This raises in the FrozenList case, but not the FrozenNDArray case. So I move the bytes test to test_frozen.py and have seperate tests for FrozenList and FrozenNDArray.

@topper-123 topper-123 force-pushed the topper-123:remove__bytes__ branch from e1061be to 2053dfa May 18, 2019

@gfyoung gfyoung requested a review from jreback May 18, 2019

def test_bytestring_with_unicode(self):
df = DataFrame({'A': ["\u05d0"]})
bytes(df)
def test_str_to_bytes_raises(self):

This comment has been minimized.

Copy link
@WillAyd

WillAyd May 19, 2019

Member

I think OK to just remove these tests; error message just matches stdlib

This comment has been minimized.

Copy link
@topper-123

topper-123 May 19, 2019

Author Contributor

Why not just keep them? these things are now (after this PR) supposed to raise,so just have a test for that?

This comment has been minimized.

Copy link
@WillAyd

WillAyd May 19, 2019

Member

I'm not hugely tied to it but these tests are making assertions about things that the stdlib does instead of something pandas explicitly does, so just seems strange to me that have that in our test suite. Let's see what others think

This comment has been minimized.

Copy link
@topper-123

topper-123 May 19, 2019

Author Contributor

Ok, I see your point. Not a hard point for me either. I can remove them later tonight.

This comment has been minimized.

Copy link
@jreback

jreback May 19, 2019

Contributor

i think these tests are fine; pretty innocuous and prevent adding back a __bytes__.

@jreback jreback added this to the 0.25.0 milestone May 19, 2019

@jreback

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

these tests are ok, no big deal that we have a slight duplication with the std library.

@jreback jreback merged commit 420c127 into pandas-dev:master May 19, 2019

11 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190518.19 succeeded
Details
pandas-dev.pandas (Checks_and_doc) Checks_and_doc succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

thanks @topper-123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.