Skip to content

Fix make_signature TypeError in py3 #17609

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 22, 2017
Merged

Fix make_signature TypeError in py3 #17609

merged 4 commits into from
Sep 22, 2017

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Sep 21, 2017 •
edited by jreback
Loading

xref #17608

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

jreback
jreback requested changes Sep 21, 2017
View reviewed changes
pandas/tests/util/test_util.py Outdated

def test_make_signature():
# See GH 17608
_ = make_signature(validate_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to assert something here

Copy link
Member Author

Choose a reason for hiding this comment

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

This test will fail under the status quo.

@codecov
Copy link

codecov bot commented Sep 21, 2017

Codecov Report

Merging #17609 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17609      +/-   ##
==========================================
+ Coverage   91.19%    91.2%   +<.01%     
==========================================
  Files         163      163              
  Lines       49625    49625              
==========================================
+ Hits        45257    45260       +3     
+ Misses       4368     4365       -3
Flag Coverage Δ
#multiple 88.99% <100%> (+0.02%) ⬆️
#single 40.2% <0%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/util/_decorators.py 78% <100%> (+12%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.77% <0%> (-0.1%) ⬇️

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 fedf922...7cdf3b1. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 21, 2017 •
edited
Loading

Codecov Report

Merging #17609 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17609      +/-   ##
==========================================
+ Coverage   91.19%    91.2%   +<.01%     
==========================================
  Files         163      163              
  Lines       49625    49652      +27     
==========================================
+ Hits        45257    45285      +28     
+ Misses       4368     4367       -1
Flag Coverage Δ
#multiple 88.99% <100%> (+0.02%) ⬆️
#single 40.18% <0%> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/util/_decorators.py 78% <100%> (+12%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/io/excel.py 80.37% <0%> (-0.18%) ⬇️
pandas/core/frame.py 97.77% <0%> (-0.1%) ⬇️
pandas/tseries/offsets.py 97% <0%> (ø) ⬆️
pandas/core/categorical.py 95.59% <0%> (+0.01%) ⬆️
pandas/io/formats/format.py 96.07% <0%> (+0.02%) ⬆️
pandas/core/indexes/range.py 92.83% <0%> (+0.24%) ⬆️

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 fedf922...943b3e1. Read the comment docs.

@jbrockmendel
Copy link
Member Author

Travis error looks unrelated:

RuntimeError: module compiled against API version 0xb but this version of numpy is 0xa
Traceback (most recent call last):
  File "/home/travis/miniconda3/envs/pandas/lib/python3.6/site-packages/_pytest/config.py", line 342, in _getconftestmodules
    return self._path2confmods[path]
KeyError: local('/home/travis/build/pandas-dev/pandas/pandas')

gfyoung
gfyoung reviewed Sep 21, 2017
View reviewed changes
pandas/tests/util/test_util.py Outdated
make_signature(validate_kwargs)
# Case where the func does not have default kwargs
make_signature(deprecate_kwarg)
# Case where the func does have default kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Can you move your comments above the test you're running?

jreback
jreback reviewed Sep 21, 2017
View reviewed changes
pandas/tests/util/test_util.py Outdated
def test_make_signature():
# See GH 17608
# Case where the func does not have default kwargs
make_signature(validate_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this doing now and what does this fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

The case on line 476 raises a TypeError in py3. See #17608.

Copy link
Contributor

Choose a reason for hiding this comment

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

this works for me in py3 now, so its not a good enough example

Copy link
Member Author

Choose a reason for hiding this comment

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

Before I dig into this, did you run make_signature with both cases in the test? My last comment had a typo, should have said line 477.

Copy link
Contributor

Choose a reason for hiding this comment

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

i ran make_signature(validate_kwargs)

Copy link
Member Author

Choose a reason for hiding this comment

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

pls try the call in the next line make_signature(deprecate_kwarg)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok this is fine. but still put an expected result and test against that, otherwise lgtm.

@jreback jreback added this to the 0.21.0 milestone Sep 22, 2017
@jreback
Copy link
Contributor

jreback commented Sep 22, 2017

lgtm ping on green.

@jbrockmendel
Copy link
Member Author

ping. pls consider re-opening #17600, as it fixes a problem that caused this to fail silently.

@jreback jreback merged commit e6d8953 into pandas-dev:master Sep 22, 2017
@jreback
Copy link
Contributor

jreback commented Sep 22, 2017

thanks. see my comments for #17600

@jbrockmendel jbrockmendel deleted the fix_make_sig branch October 30, 2017 16:23
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
@jbrockmendel @alanbato
Fix make_signature TypeError in py3 (pandas-dev#17609)
e9ccbfc
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@jbrockmendel @No-Stream
Fix make_signature TypeError in py3 (pandas-dev#17609)
c35d288
@jbrockmendel jbrockmendel mentioned this pull request Feb 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfyoung gfyoung gfyoung left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @jreback @gfyoung