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

Bugfix: Couldn't subclass Panel #888

Closed
wants to merge 6 commits into from
Closed

Conversation

joshuaar
Copy link
Contributor

@joshuaar joshuaar commented Mar 9, 2012

I was trying to create a subclass from Panel and came into this snag. I'm not sure if there was a reason Panel was using a hard coded _constructor method, but I changed it to be more generic. I'm new to open source, so constructive criticism is welcome. Am I doing this right?

Josh added 3 commits March 8, 2012 22:36
various methods were calling the Panel constructor instead of the
_constructor method. Also from_dict was returning Panel objects
instead of __class__ objects.
Panel instead of self.__class__ to construct modified objects.
@@ -320,7 +320,7 @@ def from_dict(cls, data, intersect=False, orient='items', dtype=None):
data, index, columns = _homogenize_dict(data, intersect=intersect,
dtype=dtype)
items = Index(sorted(data.keys()))
return Panel(data, items, index, columns)
return __class__(data, items, index, columns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be cls rather than __class__?

@takluyver
Copy link
Contributor

Welcome to open source! You're on the right lines, I've just highlighted a few potential problems. Other people who're more familiar with the design of panel may give you more suggestions. Remember, although the comments might come across as quite terse, we are grateful that you're using and improving the code.

@joshuaar
Copy link
Contributor Author

joshuaar commented Mar 9, 2012

Thanks for the feedback. I changed _constructor to return type(self), and the _constructorGroupBy thing was a consequence of sloppy search and replace, so I fixed that. Should I submit another request or keep this one?

@adamklein
Copy link
Contributor

Merged in. Welcome to the project!

Future reference, it's easier (on you) if you work in a non-master branch. When you update, you'll have to "git reset --hard /master"

@adamklein adamklein closed this Mar 14, 2012
@wesm
Copy link
Member

wesm commented Mar 15, 2012

@joshuaar a more fundamental question is: why are you subclassing Panel? Often when people ask me about subclassing I talk them out of it (viz "favor composition over inheritance")

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

Successfully merging this pull request may close these issues.

4 participants