Skip to content

NotImplementedError messages doc update #7872 #7899

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
Closed
wants to merge 3 commits into from
Closed

NotImplementedError messages doc update #7872 #7899

wants to merge 3 commits into from

Conversation

ilam
Copy link

@ilam ilam commented Aug 1, 2014

closes #7872

I have updated the NonImplementedError messages #7872 in all places applicable.
It includes changes to statements of type

  • raise NotImplementedError
  • raise NotImplementedError()
    Comment on the changes.

The one place I have doubt and not sure whether to add one or not would be https://github.com/pydata/pandas/blob/master/.coveragerc#L17

@ilam ilam mentioned this pull request Aug 1, 2014
Closed
@jreback jreback added this to the 0.15.0 milestone Aug 1, 2014
@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

don't worry about the coverage file

jreback
jreback reviewed Aug 1, 2014
View reviewed changes
pandas/core/generic.py
@@ -2179,7 +2179,7 @@ def fillna(self, value=None, method=None, axis=0, inplace=False,
raise ValueError('must specify a fill method or value')
if self._is_mixed_type and axis == 1:
if inplace:
raise NotImplementedError()
raise NotImplementedError("fillna with inplace=True and _is_mixed_type=True")
Copy link
Contributor

Choose a reason for hiding this comment

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

and axis=1

@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

pls squash the commits to one, see here: https://github.com/pydata/pandas/wiki/Using-Git

@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

@hayd @cpcloud @jorisvandenbossche

usually for an abstract type class I would NotImplementedError; is this standard? anything else to indicate that its an abc method?

@cpcloud
Copy link
Member

cpcloud commented Aug 1, 2014

there's the abc module:

from abc import abstractmethod, ABCMeta


class CannotInstantiate(object):
    __metaclass__ = ABCMeta

    @abstractmethod
    def hardy_har(self):
        pass

This will raise TypeError: Can't instantiate abstract class CannotInstantiate with abstract methods hardy_har
What's a little annoying is that you can't use abstractmethod without using the ABCMeta as a metaclass and Python 3 has different and incompatible syntax for specifying a metaclass

😡

@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

ok maybe should then have the not implemented errors signify that they are abstract

e.g. in indexing, the _LocationIndexer is an abstract class. something like "NotImplemenetedError('this is an abstract class ......")`` mainly to signal to a sub-class implementer. like a convention.

or the message like you did above

@jreback jreback closed this Aug 1, 2014
@jreback jreback reopened this Aug 1, 2014
@ilam
Copy link
Author

ilam commented Aug 1, 2014

All the commits are squashed into one commit. Next time, I will careful to do the same before pushing it to the public repo.

@hayd
Copy link
Contributor

hayd commented Aug 1, 2014

Thanks for going through this! Part of the reason for the original issue is that the error message is it should say why not just where/what.... Unfortunately, I can't find any specific advice online about writing good exceptions messages. :( I do think these ought to be proper sentences...

IMO it would be nice if abstract methods were reflected in the error message (confession: I also do this)... Would it be easier/compat solution to define an AbstractMethodError <: NotImplementedError (whose message is (or is prefixed with) "This method must be defined on the concrete class" or something)? :s

@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

@hayd thats a great idea!

@ilam want to see if you can define (prob in core/common.py) and change ones that are Abstract classes (i'll look thru after)

@ilam
Copy link
Author

ilam commented Aug 1, 2014

@jreback Sure. As far as I understand, instead of raising NotImplementedError, instead raising AbstractMethodError (after defining it) with the appropriate message would be good. Right?

@jreback
Copy link
Contributor

jreback commented Aug 1, 2014

@ilam exactly. If you have about whether certain classes should be abstract or not, pls post a link here and we can discuss.

@ilam
Copy link
Author

ilam commented Aug 1, 2014

@jreback Is this the right way of doing it? 8e5ca6f. Updated ae8b15c

@ilam
Copy link
Author

ilam commented Aug 2, 2014

@jreback Sure, I am sorry, I made that mistake again, I will squash them, and include here a list of Abstract classes, if any for which these changes might be required.

@ilam
Copy link
Author

ilam commented Aug 2, 2014

https://github.com/pydata/pandas/blob/master/pandas/core/indexing.py#L58
Classes _LocationIndexer, _NDFrameIndexer, _iLocIndexer are all abstract classes and have the NotImplementedError in them.

@jreback
Copy link
Contributor

jreback commented Aug 2, 2014

yep

@ilam
Copy link
Author

ilam commented Aug 2, 2014

Cool. Will change them to AbstractMethodError in those abstract classes.

jreback
jreback reviewed Aug 2, 2014
View reviewed changes
pandas/core/base.py
@@ -382,7 +382,7 @@ def _box_func(self):
"""
box function to get object from internal representation
"""
raise NotImplementedError
raise NotImplementedError("Box function to get object from internal representation")
Copy link
Contributor

Choose a reason for hiding this comment

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

these are both abstract as well

jreback
jreback reviewed Aug 2, 2014
View reviewed changes
pandas/core/groupby.py
@@ -3172,7 +3172,7 @@ def _chop(self, sdata, slice_obj):
return sdata.iloc[slice_obj]

def apply(self, f):
raise NotImplementedError
raise NotImplementedError("DataSplitter apply function")
Copy link
Contributor

Choose a reason for hiding this comment

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

abstract

@hayd
Copy link
Contributor

hayd commented Aug 12, 2014

@cpcloud solution for the python 2/3 meta class thing is to copy six's with_metaclass into compat http://stackoverflow.com/a/18513858/1240268 using @abstractmethod would be the best solution!

@jreback jreback modified the milestones: 0.15.1, 0.15.0 Sep 9, 2014
@jreback
Copy link
Contributor

jreback commented Sep 9, 2014

@ilam want to rebase & update?

@jreback
Copy link
Contributor

jreback commented Jan 18, 2015

@ilam can you rebase and revisit this?

@jreback jreback modified the milestones: 0.16.0, Next Major Release Mar 3, 2015
@jreback jreback mentioned this pull request Apr 8, 2015
Closed
hoh added a commit to hoh/pandas that referenced this pull request Apr 13, 2015
@hoh
FIX: Describe NotImplementedErrors or use AbstractMethodError
52ab99b 
This issue fixes pandas-dev#7872 by either describing the reason why a NotImplementedError is raised,
or by throwing an AbstractMethodError in the case of abstract methods (eg: Mixins) that
should be overwritten by the inheriting classes.

This issue is based on discussion of PR pandas-dev#7899 which was never merged.
@hoh hoh mentioned this pull request Apr 13, 2015
Closed
jreback pushed a commit that referenced this pull request Apr 14, 2015
@hoh @jreback
FIX: Describe NotImplementedErrors or use AbstractMethodError
8fe1cf6 
This issue fixes #7872 by either describing the reason why a NotImplementedError is raised,
or by throwing an AbstractMethodError in the case of abstract methods (eg: Mixins) that
should be overwritten by the inheriting classes.

This issue is based on discussion of PR #7899 which was never merged.
@jreback
Copy link
Contributor

jreback commented Apr 14, 2015

replaced by #9887

@jreback jreback closed this Apr 14, 2015
@jreback jreback modified the milestones: 0.16.1, Next Major Release Apr 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Docs Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.16.1
Development

Successfully merging this pull request may close these issues.

Give all NotImplementedErrors a description
5 participants
@ilam @jreback @cpcloud @hayd @sinhrks