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

Remove encouragement to author a base class for all Exception subclasses in a module #78719

Closed
nathanielmanistaatgoogle mannequin opened this issue Aug 28, 2018 · 18 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir easy type-feature A feature request or enhancement

Comments

@nathanielmanistaatgoogle
Copy link
Mannequin

BPO 34538
Nosy @malemburg, @gvanrossum, @freddrake, @brettcannon, @rhettinger, @methane, @nathanielmanistaatgoogle, @ammaraskar, @hugovk, @miss-islington
PRs
  • bpo-34538: Remove Exception subclassing from tutorial #30361
  • [3.10] bpo-34538: Remove Exception subclassing from tutorial (GH-30361) #30374
  • [3.9] bpo-34538: Remove Exception subclassing from tutorial (GH-30361) #30375
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gvanrossum'
    closed_at = <Date 2022-01-03.23:21:11.077>
    created_at = <Date 2018-08-28.20:40:51.096>
    labels = ['easy', '3.9', '3.10', '3.11', 'type-feature', 'docs']
    title = 'Remove encouragement to author a base class for all Exception subclasses in a module'
    updated_at = <Date 2022-01-03.23:21:11.076>
    user = 'https://github.com/nathanielmanistaatgoogle'

    bugs.python.org fields:

    activity = <Date 2022-01-03.23:21:11.076>
    actor = 'iritkatriel'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2022-01-03.23:21:11.077>
    closer = 'iritkatriel'
    components = ['Documentation']
    creation = <Date 2018-08-28.20:40:51.096>
    creator = 'Nathaniel Manista'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34538
    keywords = ['patch', 'easy']
    message_count = 18.0
    messages = ['324286', '324302', '324312', '324317', '324344', '324388', '324390', '324392', '324405', '324408', '325069', '326771', '326772', '326804', '326805', '409629', '409636', '409637']
    nosy_count = 11.0
    nosy_names = ['lemburg', 'gvanrossum', 'fdrake', 'brett.cannon', 'rhettinger', 'methane', 'docs@python', 'Nathaniel Manista', 'ammar2', 'hugovk', 'miss-islington']
    pr_nums = ['30361', '30374', '30375']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34538'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @nathanielmanistaatgoogle
    Copy link
    Mannequin Author

    https://docs.python.org/3.8/tutorial/errors.html (and all other versions of that page back at least as far as 2.7) currently contain the guidance "When creating a module that can raise several distinct errors, a common practice is to create a base class for exceptions defined by that module, and subclass that to create specific exception classes for different error conditions: <code example>".

    It may have seemed like a good idea at the time, but we now know from years of experience that this is an experiment that didn't pan out and we can consider this guidance a Now-Contraindicated Best Practice Of Yesteryear™.

    Modules define subclasses of Exception for lots of reasons. Some of those subclasses have no relation to one another except that they are subclasses of Exception. Some of those subclasses define Exception instances that are never raised by code in the module, but that are expected to be raised by code passed to and called by the module.

    Yes, there are times when a common base class may be appropriate - such as when an inheritance hierarchy and polymorphism that satisfy the Liskov Substitution Principle make sense for the Exception subclasses, and when the base class itself is used (such as when the base class is an item in the Raises: section of a function's doc string). But these cases aren't so common as to justify the advice being offered generally about all Exception subclass definitions.

    Exception subclasses are just classes. Advising that authors may wish to define a common base class for all Exception subclasses in a module is like advising authors that they may wish to define a common base class for all object subclasses in a module - it's very, very, very occasionally a good idea in the particular circumstances of a particular module's implementation, but very generally not.

    @nathanielmanistaatgoogle nathanielmanistaatgoogle mannequin added 3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement labels Aug 28, 2018
    @methane
    Copy link
    Member

    methane commented Aug 29, 2018

    I agree with you.

    @rhettinger
    Copy link
    Contributor

    I think the advice should stay as-is. In my experience as a teacher, the possibility doesn't usually occur to people without it having been suggested. Also, it mirrors practices in the standard library (decimal.DecimalException and sqlite3.DatabaseError). For users, it provides a way to catch possible errors that are specific to a particular module.

    @methane
    Copy link
    Member

    methane commented Aug 29, 2018

    Also, it mirrors practices in the standard library (decimal.DecimalException and sqlite3.DatabaseError).

    As Nathaniel said, "it's good idea in the particular circumstances of a particular module's implementation, but generally not." (I removed "very" because it is too noisy)

    For example, socket.error was migrated into OSError.
    So not having base exception class for module is also "practices in the standard library".

    For users, it provides a way to catch possible errors that are specific to a particular module.

    For tutorial readers, caching errors specific to particular cause should be suggested, instead of particular module.

    "How/When custom base exception class is useful" is very high level topic.
    It's too difficult for tutorial.

    If tutorial reader start using base exception class without any real benefit, it will lead ugly code like this:

    try:
    # some code here
    except ValueError as e:
    raise CustomValueError(e.args)
    except TypeError as e:
    raise CustomTypeError(e.args)
    except ...

    @brettcannon
    Copy link
    Member

    I think the question is how often in real code to people want to catch all exceptions produced by a package or module but not any others. Perhaps it would be better to re-word the advice that when there are many related exceptions that it is suggested you have a base class for them?

    @rhettinger
    Copy link
    Contributor

    See https://bugs.python.org/issue443559 and "git log -p 13af428".

    One other example from real code: requests.RequestException

    @brettcannon
    Copy link
    Member

    I'm not questioning if people have ever created a base exception for an entire package, I'm just asking how often it's actually used when the base exception didn't make sense outside of the rule-of-thumb Nathaniel is pointing out?

    For instance, it could makes sense in requests' case to have a base exception to help facilitate catching network-related errors but let through e.g. TypeError. But does that extend to most packages such that users regularly write an 'except' clause catching that package's common base exception type? That's my question and I personally don't have an answer beyond reflecting on this and not really remembering a case where I have (I _can_ remember following this pattern simply because it's habit at this point for some unknown reason :) .

    @ammaraskar
    Copy link
    Member

    @methane
    Copy link
    Member

    methane commented Aug 31, 2018

    I didn't claim this pattern is not used anymore.
    My point is "should we suggest this pattern for tutorial readers?"

    • Is this pattern recommended blindly? -- I think no.
    • How / when this pattern is recommended? -- When there is use case people want to catch the base exception.
    • Should it be in tutorial? -- I doubt it. This topic requires some experience to understand.

    @methane
    Copy link
    Member

    methane commented Aug 31, 2018

    https://github.com/search?q=%22except+TemplateError%22&type=Code

    For example, I found flask_mako's TemplateException in this search result.

    Strictly speaking, this is not base exception class. It is wraps exception during template rendering. But "why this class is useful?" is very similar to base exception class.

    It provides better traceback for mako template. They use this class for "particular reason", not because it's generally recommended practice.

    And this "particular reason" shouldn't be in Python tutorial, clearly.

    ---

    If we really need exception class hierarchy in tutorial, I think OSError is the best example. When opening a file, `except OSError:` is much better than `except (PermissionError, FileNotFound, ...)`. It's the most clear example when common base class for some distinct exceptions is useful.

    @nathanielmanistaatgoogle
    Copy link
    Mannequin Author

    I’d like to try to steer this conversation back toward what I think is the actionable question: “does the exemplification of this practice in the Errors and Exceptions portion of The Python Tutorial bring about a net benefit or a net cost to its intended audience?”.

    What is the benefit? It is that when an author happens to be authoring a module *all user-defined Exception subclasses of which satisfy some more-specific-than-Exception type* the author is led to define an intermediate class between Exception and the directly-used user-defined Exception subclasses capturing that type in a usable code element.

    What readers of the tutorial enjoy this benefit? Pretty much only those authors (who happen to be authoring a module *all user-defined Exception subclasses of which satisfy some more-specific-than-Exception type*) who are still learning about classes and inheritance. That’s a doubly slim population, isn’t it? Maybe also those who kind of know, but aren’t so sure and could use some reinforcement from seeing in the tutorial something that they independently did on their own in their own code. I wouldn’t think that authors who already know with confidence and from experience about classes and inheritance would benefit from the example in the tutorial, so “In my experience as a teacher, the possibility doesn't usually occur to people without it having been suggested” comes as a surprise to me. But… okay, them too - but again, only when they happen to be authoring a module *all user-defined Exception subclasses of which satisfy some more-specific-than-Exception type*.

    What is the cost? It is that when an author happens to be authoring a module that does *not* have the property that all user-defined Exception subclasses satisfy some more-specific-than-Exception type, the common intermediate class is just bloat. It’s noise disrupting the signal. It undermines the API design advice “when in doubt, leave it out”. It unnecessarily widens the module’s API. It undermines the API design advice “classes are the most complex kind of code element in common use so they have the highest value proposition to satisfy to justify their existence”. It violates the Liskov Substitution Principle. Maybe most importantly, it obscures other relationships among the user-defined Exception subclasses in the module such as a superclass shared by some-but-not-all of the module’s user-defined Exception subclasses, if such other relationships exist.

    What readers of the tutorial pay this cost? Those who are still learning the language. And those who are following pattern and convention - note that the tutorial contains only one example of user-defined Exception subclasses, and… an unfortunate fact of life of tutorials is that readers are invited to generalize from single examples. And, as I think we’ve seen in this conversation, those who just picked up the practice at one point and have kept it going.

    The Exception subclass hierarchy of sqlite3 that was mentioned earlier in this conversation demonstrates exactly this bloat and misapplication - there’s Warning, which is a direct subclass of Exception, and there’s Error, which is also a direct subclass of Exception and has the erroneous specification “The base class of the other exceptions in this module”, and there’s DatabaseError, which is a subclass of Error, and then there are IntegrityError, ProgrammingError, OperationalError, and NotSupportedError, which inherit from DatabaseError. What’s the use of Error? There are no code elements in the module specified as raising Error. There’s example code in the module showing “except sqlite3.Error”, but why shouldn’t that be “except sqlite3.DatabaseError”?

    It’s a red herring is that the practice appears widely applied in existing Python code - well of course; it’s been exemplified in the tutorial for seventeen years! :-P

    One last thing to consider: look at the example specifically - InputError and TransitionError. There’s no elsewhere-in-the-module example code showing a function that has “Error” in its “Raises:” specification and could raise either an InputError or a TransitionError, and there’s no outside-of-the-module example code showing a user of the module calling a module function and saving duplicated lines of code because they want to respond to InputErrors and TransitionErrors in exactly the same way.

    We should remove the “Base class for exceptions in this module” Error class from the tutorial’s example because it just isn’t beneficial enough, in enough applicable modules, to enough authors, and it’s more than costly enough, in enough modules to which it doesn’t apply, and to enough authors, even just as noise and API bloat. I don’t know that this could have been calculated from first principles seventeen years ago; I think perhaps it took the experience of having the guidance out there, so rarely merited and so widely implemented, to see it being a net drag.

    @methane
    Copy link
    Member

    methane commented Oct 1, 2018

    Thanks, Nathaniel. I totally concur with you.

    This is tutorial section. We should focus on readers of the tutorial.

    @rhettinger
    Copy link
    Contributor

    Guido, the check-in message for this section indicates that Fred Drake added this wording at your behest. Do you still agree with the guidance and examples or would you like to have it removed from all active versions of the documentation as proposed?

    https://docs.python.org/3.8/tutorial/errors.html#user-defined-exceptions

    @rhettinger rhettinger assigned gvanrossum and unassigned docspython Oct 1, 2018
    @gvanrossum
    Copy link
    Member

    I think as a general recommendation it is not such a good idea that we should specifically mention it. (Is it in PEP-8 too? If so it should be removed there too.)

    It's a pattern that is sometimes helpful, sometimes not. I don't think that people need to hear about it from the official docs about exceptions. People can learn from the standard exception hierarchy that sometimes it's useful to have a common base class *for exceptions that are related in some way*, in particular if there would be a reason to catch all of them with the same handler.

    So I'm in agreement with Nathaniel M here.

    @malemburg
    Copy link
    Member

    Just as extra data point:

    It is fairly common to have a common exception class which is then used a mixin class together with the standard exception classes, so that you can indeed identify the source of an exception and catch errors based on the source (e.g. say you want to catch database errors coming from MySQL specifically).

    The Python DB-API also requires to create a separate hierarchy for this purpose.

    Overall, I wouldn't call this a non-best practice. It depends on the use case, whether it's useful or not.

    @iritkatriel iritkatriel added easy 3.9 only security fixes 3.10 only security fixes and removed 3.7 (EOL) end of life labels Mar 14, 2021
    @iritkatriel iritkatriel added 3.11 only security fixes and removed 3.8 only security fixes labels Jan 3, 2022
    @miss-islington
    Copy link
    Contributor

    New changeset 2db5613 by Hugo van Kemenade in branch 'main':
    bpo-34538: Remove Exception subclassing from tutorial (GH-30361)
    2db5613

    @miss-islington
    Copy link
    Contributor

    New changeset 0b3c3cb by Miss Islington (bot) in branch '3.10':
    bpo-34538: Remove Exception subclassing from tutorial (GH-30361)
    0b3c3cb

    @miss-islington
    Copy link
    Contributor

    New changeset 4affb99 by Miss Islington (bot) in branch '3.9':
    bpo-34538: Remove Exception subclassing from tutorial (GH-30361)
    4affb99

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants