python / cpython

The Python programming language
https://www.python.org
Other
63.27k stars 30.29k forks source link

contextlib.suppress should capture exception for inspection and filter on substrings #77327

Open jaraco opened 6 years ago

jaraco commented 6 years ago
BPO 33146
Nosy @rhettinger, @jaraco, @ncoghlan, @serhiy-storchaka

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: ```python assignee = None closed_at = None created_at = labels = ['3.8', 'type-feature', 'library'] title = 'contextlib.suppress should capture exception for inspection and filter on substrings' updated_at = user = 'https://github.com/jaraco' ``` bugs.python.org fields: ```python activity = actor = 'ncoghlan' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'jaraco' dependencies = [] files = [] hgrepos = [] issue_num = 33146 keywords = [] message_count = 4.0 messages = ['314461', '314480', '314503', '314524'] nosy_count = 4.0 nosy_names = ['rhettinger', 'jaraco', 'ncoghlan', 'serhiy.storchaka'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue33146' versions = ['Python 3.8'] ```

jaraco commented 6 years ago

I propose the following expansion of the interface of contextlib.suppress. Currently, when entering the context, suppress returns None. Instead, it could return an object that provides some detail about the exception.

Inspiration for an implementation exists in pytest, capturing the commonly-encountered use-cases, where one wishes to capture, suppress, and then act on a subset of exceptions, allowing others to raise normally.

In py-181, I suggest exposing this functionality generally, but others had an instinct similar to mine - that perhaps the stdlib should be providing this interface.

In addition to saving the exception for inspection, the pytest implementation also allows a "message" to be supplied (for those exceptions where only some subset of the class of Exception is suppressed).

I present this concept here for consideration and feedback. Can contextlib.suppress be expanded with such an interface?

rhettinger commented 6 years ago

What kind useful information to you expect to get before an exception is raised?

For example:

    with suppress(FileNotFoundError) as e:
         os.remove(somefile)

What could *e* possibly be that would be useful. AFAICT, all we know at the time of __enter__ is the exception class. No new information was created by the context manager call.

Ideally, it would be great is this API were to remain simple. User code would likely be more clear if any other logic were done outside the context manager rather than happening indirectly and inexplicitly in the suppress call. To me, "suppress" means suppress -- it doesn't mean capture and analyze that which is ignored. So, at first glance, this seems like a mix of feature creep and mission creep.

serhiy-storchaka commented 6 years ago

I'm not sure that contextlib.suppress() should be added at first place.

try:
    os.remove(somefile)
except FileNotFoundError:
    pass

is a tiny bit longer, but is more explicit, doesn't depend on the other module, works in all Python versions that have this exception, and is easily extensible.

try:
    os.remove(somefile)
except FileNotFoundError as e:
    # what do you want to do with e?
    pass
ncoghlan commented 6 years ago

I wouldn't expand the scope of contextlib.suppress, since it has a pretty clear purpose: pretend the exception never happened. (This was even clearer with the original "ignored" name, before I was talked into changing it to use the current more technically correct, but far less intuitive, name - a decision I still regret).

However, when writing tests, or otherwise introspecting thrown exceptions (i.e. in programs about programs), it's not uncommon for me to want a construct like:

    with catch(FileNotFoundError) as err:
        os.remove(somefile)

    if err.caught is not None:
        ...

The benefit over the try/catch form is similar to the benefits of suppress:

  1. Up front declaration that that kind of exception isn't going to escape and execution will continue after the suite
  2. More efficient use of vertical whitespace (one extra line instead of three)

In this case, we could also end up expanding the API to provide a better building block for testing tools like "assertRaises" and "assertRaisesRegex" by accepting a "filter" callback argument that receives the caught exception, and can either return False to suppress it, True to reraise it, or explicitly raise a different exception to replace it. (That suggested callback API is deliberately similar to an __exit__ implementation that only accepts the exception value, rather than the full type/value/traceback triple).

(The simpler name may mean that folks end up preferring catch() to suppress() even when they don't need the result of __enter__. I'd be OK with that outcome).

iritkatriel commented 2 years ago

The unit test use case is covered by assertRaises now. Is there still a need for this?

jaraco commented 2 years ago

Thanks Irit for reviving this discussion.

What could e possibly be that would be useful. AFAICT, all we know at the time of __enter__ is the exception class. No new information was created by the context manager call.

e is a mutable object that contains details about a relevant exception that was raised. It probably also has some features to facilitate its use. See ExceptionTrap for a possible implementation (that doesn't at this time have the "filter by message" behavior).

To me, "suppress" means suppress -- it doesn't mean capture and analyze that which is ignored.

Good point. Maybe suppress isn't the right name for such an object, but I do observe that RaisesContext and ExceptionTrap have the same basic behavior as suppress, so suppress becomes the degenerate form of those implementations. In other words, suppress = random.choice([RaisesContext, ExceptionTrap]) would not violate any uses of suppress.

The unit test use case is covered by assertRaises now. Is there still a need for this?

I think you mean the unittest use case. assertRaises is a feature of unittest the module but not unit testing in general (such as pytest or nose might need). Additionally, my original report doesn't specifically mention unit testing and Nick has specifically mentioned the extra unit test scope.

otherwise introspecting thrown exceptions (i.e. in programs about programs)

So no, the use case is definitely not covered by assertRaises, although it might have been nice for assertRaises to have been built on a more foundational implementation that satisfies its use case and others outside unittest.

Consider this example, where one wishes to capture a possible exception and return the outcome (raised or not) as the result of a function. Without ExceptionTrap, the user is responsible for constructing their own context object, trapping the exception, and then conditionally storing the exception in the context object.

It would be convenient if Python provided a tested, proven implementation of that pattern, and it just so happens that if Python provided such an implementation, its simplest form might obviate suppress.

serhiy-storchaka commented 2 years ago

What testing framework do you use that has no analogue of assertRaises()?

ambv commented 1 year ago

https://docs.pytest.org/en/stable/reference/reference.html#pytest.raises is the equivalent for pytest.

I would prefer to keep suppress() as "ignore", which was the original intent of Nick's. I feel like any modality introduced to such a trivial convenience construct muddles the reason to use it in the first place.

jaraco commented 1 year ago

What testing framework do you use that has no analogue of assertRaises()?

One testing framework that could benefit from this proposed functionality is doctests.

However, this bug isn't about simply supporting tests. This bug is about supporting the general use-case where an exception or subset of an exception needs to be caught and retain some information about what was caught, which can happen outside of tests. My original motivation as linked above was to make the functionality that's available in pytest available outside pytest.

Consider for example this use case, where a routine is iterating over filenames and wishes to capture details about the items that fail to process.

Moreover, it would be nice if unittest assertRaises and pytest's raises were built on the same foundational behavior such that the assert aspect is composed with the generalized exception capture logic.

I would prefer to keep suppress() as "ignore"...

Agreed, it does complicate this otherwise simply construct, but it still leaves me wanting for more sophisticated behavior.

Without supplying this common behavior somewhere in the stdlib, each test framework needs to implement their own and those not running in a test framework need to invent their own, cargo cult the behavior, or pull in a dependency for this otherwise isolated behavior.

Would you consider a contextlib.trap or contextlib.catch or similar?