On Tue, Jun 18, 2024 at 11:54 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote: > > > @@ -15,18 +18,34 @@ class InteractiveShell(SingleActiveInteractiveShell): > > > + def _start_application(self, get_privileged_command: Callable[[str], > > str] | None) -> None: > > + """Overrides :meth:`_start_application` in the parent class. > > + > > + Add a weakref finalize class after starting the application. > > + > > + Args: > > + get_privileged_command: A function (but could be any callable) > > that produces > > + the version of the command with elevated privileges. > > + """ > > + super()._start_application(get_privileged_command) > > + self._finalizer = weakref.finalize(self, self._close) > > I think we can just add the above line to start_application() to achieve > the same thing. And we should move the docstring to the public method.
Sure, makes sense to me, we only need the finalizer when we start manually anyway, there's no need to set it up when you use it as a context manager. Actually, I wonder if this would throw an exception at the time of garbage collection if you used an InteractiveShell as a context manager. I think it might because the context manager doesn't trigger the finalizer, so it probably would try to clean up twice. Good catch! > > > + > > def start_application(self) -> None: > > """Start the application.""" > > self._start_application(self._get_privileged_command) > >
