Despite the fact that the fix is simple, I love to see this kind of explanation in the PR first message.
Thanks for spending time to do it in this way.

👍 for me.

@damianavila: Eh, it saves time in the end ;-)

After the discussion in #6611, this makes sense to me. I'd like to hear from @takluyver on the implementation.

Let's not push it into run_code - that's the very innermost core of IPython. Is it practical to do it using a post_execute callback?

Is it practical to do it using a post_execute callback?

I don't see a problem... probably safer than the current one.
Let's see what @njsmith think about that...

Is it practical to do it using a post_execute callback?

That should be fine. Just register it as a hook during initialization.

There are three separate issues here: (a) to use a hook or just to hard-code it in the file, (b) whether to clear the registry before or after the code runs, (c) whether to clear the registry in run_cell (*_execute hooks) or run_code (some sort of legacy hook system?).

Re: (a): I don't see much point in decoupling for the sake of decoupling -- there's a tradeoff between the benefits of flexibility and the costs of reduce locality making it harder to read and understand the code. In this case, the __warningregistry__ thing isn't something that needs to be configured -- it should happen unconditionally every time we execute some code, so I just put it in the code. This might seem less flexible, but IME it's easier to edit a hardcoded bit source than to disable a hook -- it takes less grepping to find the source itself than to find where a hook is registered :-). But, you know, this is basically a taste issue, and it's your codebase, so!

Re: (b): IMO it's better to clear the __warningregistry__ before executing code rather than after. If our clearing code gets called every single time we execute code, then it doesn't matter whether we do the clearing pre_ or post_ execution. But, it's also possible that some code will be run without our knowledge and pollute the __warningregistry__ with spurious entries. (This is esp. true if we move the logic into run_cell. I don't know under what circumstances people use run_cell vs run_ast_nodes vs run_code, but anyone using the latter two will skip the logic in run_cell.) So if we use a hook we should use a pre_* hook, etc.

Re: (c): My reasoning for pushing the registry clearing down into run_code instead of just run_cell was that every call to exec creates a new line number scope and invalidates the entries in __warningregistry__, so we should clear the registry every time we call exec. And run_code is where we call exec. Pushing it further out creates more situations where code could be run with spurious entries in the __warningregistry__.

So, that's why I wrote the patch the way I did :-). If on reflection you'd prefer something else let me know.

Thanks for explaining your reasoning:

• I would still like to do this with a hook, just for the sake of decoupling. Clearing the warnings registry is not fundamental to running code, and that part of the codebase is already more than complex enough without adding extra concerns to it.
• In terms of when people use the different methods: I promote only run_cell as something that third parties should use. There might be third parties using run_ast_nodes or run_code, but I would consider those corner cases. All our own uses, as far as I recall, go through run_cell.
• Pre/post: Whichever you prefer. I don't think we've ever heard about code polluting __warningregistry__, and it seems hard to do accidentally, so I'm not especially concerned about that.

So I made an attempt to get this working as a hook (see the current version of this PR), and noticed two issues:

• AFAICT the only place to define and register the hook is still directly inside InteractiveShell code, and the hook code itself is a method on InteractiveShell. So the coupling is not very reduced.
• The tests are failing because there's a test which calls ip.events.reset_all(), which of course deletes my hook before my test gets a chance to run. IMO if I have a pristine InteractiveShell, then calling ip.events.reset_all() should be a no-op. But there's really no way to make this work with a default hook, unless we start poking at the definition of EventManager itself which ugh.

So..... how do you want to do this?

Registering hooks doesn't have to be done inside interactiveshell code - anything that gets a reference to the shell object (like extensions) can use shell.events.register(). In this case, since you want it on all shell objects, doing it inside interactiveshell makes sense.

Maybe the reset methods should be renamed something like clear (events is a new API, so I think we can get away with this before release). The test shouldn't really be doing reset_all(), whatever the name is - it should be taking out only the callbacks it installed. Using reset_all() there was just laziness on my part.

Going one step further, maybe reset() and reset_all() shouldn't even exist. The design of using events is precisely that you shouldn't think about what other callbacks might be triggered by an event, so killing all the callbacks on an event is generally the wrong thing to do. It's especially bad when most events aren't handled by default, so it's easy to fall into using it just to clear a callback you know about. That should probably be a separate PR, though.

@takluyver it sounds like cleaning up event reset should be a separate PR. What should we do here so this can be merged, and the other stuff investigated separately?

As a start, I went ahead and made a commit that just removes the reset APIs. This PR currently contains that commit plus the __warningregistry__ commit (click the "commits" tab if you want to review them separately). We could make them into two PRs I guess if that would make everyone happier, though if they look good then it might be simpler just to merge them both together here.

I'm fine with this as-is. @takluyver?

Yep. Thanks for your patience, @njsmith .