#20841 - Verbose not implemented errors #1569

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

Cheekio commented Sep 6, 2013

Non-Verbose 'raise NotImplementedErrors' have been been found and given useful error messages, all 94 of them (across 26 files), as discussed in https://code.djangoproject.com/ticket/20841

92 of these messages were simple base classes that had placeholder methods, and were updated with 'may/must overwrite in subclass' errors indicating the base class that required subclassing, and the method requiring overwriting. In terms of wording, 'may require' was used if the method was not essential for the use of the baseclass, whereas 'must provide' was used when it was. Mostly.

2 of these were features that seemed to be forgotten, specicially utils/regex_helper.py line number 95 and utils/dateformat.py's line number 67.

These updated NotImplementedError instances were tested using python tests/runtests.py, and give a response of 'OK'.

django/contrib/admin/filters.py
@@ -33,26 +33,26 @@ def has_output(self):
"""
Returns True if some choices would be output for this filter.
"""
- raise NotImplementedError
+ raise NotImplementedError('sublasses of ListFilter must provide a has_output() method')
@EricBoersma

EricBoersma Sep 6, 2013

Contributor

Typo.

django/contrib/auth/hashers.py
@@ -192,7 +192,7 @@ def verify(self, password, encoded):
"""
Checks if the given password is correct
"""
- raise NotImplementedError()
+ raise NotImplementedError('sublasses of BasePasswordHasher must provide a verify() method')
@EricBoersma

EricBoersma Sep 6, 2013

Contributor

Typo in all three of these methods..

django/contrib/auth/models.py
@@ -246,10 +246,10 @@ def has_usable_password(self):
return is_password_usable(self.password)
def get_full_name(self):
- raise NotImplementedError()
+ raise NotImplementedError('sublasses of ListFilter must provide a get_full_name() method')
@EricBoersma

EricBoersma Sep 6, 2013

Contributor

Also a typo here.

django/contrib/auth/models.py
@@ -442,16 +442,16 @@ def __hash__(self):
return 1 # instances always return the same hash value
def save(self):
- raise NotImplementedError
+ raise NotImplementedError('Django provides no DB representation for anonymous users objects')
@EricBoersma

EricBoersma Sep 6, 2013

Contributor

I think the verbiage that you use below (anonymous user objects) is more grammatically correct than the users objects you use in this instance.

django/contrib/gis/db/backends/base.py
@@ -101,7 +101,7 @@ def geo_db_type(self, f):
Returns the database column type for the geometry field on
the spatial backend.
"""
- raise NotImplementedError
+ raise NotImplementedError('instances of BaseSpatialOperations must provide geo_db_type() method')
@EricBoersma

EricBoersma Sep 6, 2013

Contributor

I may be missing the distinction between BaseSpatialOperations and other classes here so please help me understand if I am, but I feel like we should use universal verbiage across the board, that is, either instances or subclasses exclusively.

@Cheekio

Cheekio Sep 6, 2013

Contributor

Fixed, I was mistaken in thinking BSO was instanced directly but in existent code it is only subclassed.

Contributor

EricBoersma commented Sep 6, 2013

I'm certainly no expert on the topic, but this looks pretty good to me, with the exception of the minor nitpicks that I included in the comments.

Cheekio added some commits Sep 6, 2013

Added Verbose Error Messages for raised NotImplemented Errors, mostly…
… relating to unimplemented methods on base classes
Further standardized wording, no longer spoke of subclassing for actu…
…ally 'not implemented' features (like Swatch Internet Time)

joequery commented Sep 6, 2013

Thanks for taking care of this.

To prevent those pesky merge commits from showing up while you're trying to keep your branch up to date with master, you can rebase master onto your branch.

$ git checkout verbose_not_implemented_errors
$ git rebase master
... solve merge conflicts here.

Then if you check out master (to test the merge), you'll have a clean merge when your branch is merged in.

Owner

timgraham commented Sep 10, 2013

merged in b2b7634 - thanks!

@timgraham timgraham closed this Sep 10, 2013

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