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

ENH: Show domain and window as kwargs in repr #9536

Merged
merged 2 commits into from
Aug 10, 2017

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Aug 10, 2017

Explicit is better than implicit and all that.

Also, update the docs with this new repr - a bunch of these still printed the domain and window as floats in the default case.

Inspired a little by #9533.

It might be reasonable to not show the domain and window if either or both are the defaults

@@ -260,7 +260,7 @@ def __init__(self, coef, domain=None, window=None):
self.window = window

def __repr__(self):
format = "%s(%s, %s, %s)"
format = "%s(%s, domain=%s, window=%s)"
Copy link
Member Author

Choose a reason for hiding this comment

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

Only non-doc change is here



class TestStr(object):
def test_polynomial_str(self):
res = str(poly.Polynomial([0, 1]))
tgt = 'poly([0., 1.])'
assert_(res, tgt)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a very effective test... (from 64fce7c)

@charris
Copy link
Member

charris commented Aug 10, 2017

LGTM, although it may break downstream doctests (@mhvk, astropy) and on that account needs a mention in the release notes.

@mhvk
Copy link
Contributor

mhvk commented Aug 10, 2017

All in favour here!

@eric-wieser
Copy link
Member Author

Under Improvements?

Also, update the docs with this new repr
…nything useful

assert_ is not a substitute for assert_equal
@charris
Copy link
Member

charris commented Aug 10, 2017

Compatibility.

@charris charris merged commit 4c18530 into numpy:master Aug 10, 2017
@charris
Copy link
Member

charris commented Aug 10, 2017

Thanks Eric.

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

Successfully merging this pull request may close these issues.

None yet

3 participants