enthought / pyface

pyface: traits-capable windowing framework
Other
105 stars 55 forks source link

Remove 'parent' argument from wx StatusBarManager #1192

Closed greschd closed 1 year ago

greschd commented 1 year ago

Another small fix:

Remove the 'parent' argument from 'StatusBarManager.destroy' in the wx backend. The argument is unused, and doesn't match the interface definition.

If you prefer keeping the argument (to not break applications which may pass it explicitly?), I'd propose adding a default =None, and emitting a deprecation warning.

greschd commented 1 year ago

Good catch, I've adapted the call site now.

I wonder if we should add some kind of test? Originally, I stumbled upon this because our code triggered the following exception:

Traceback (most recent call last):
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\ui\wx\window.py", line 189, in _wx_on_close
    self.close()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\workbench\workbench_window.py", line 202, in close
    self.destroy()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\ui\wx\application_window.py", line 154, in destroy
    super().destroy()
  File "d:\ansysdev\thirdparty_sources\pyface\pyface\i_application_window.py", line 133, in destroy
    self.status_bar_manager.destroy()
TypeError: StatusBarManager.destroy() missing 1 required positional argument: 'parent'
corranwebster commented 1 year ago

I wonder if we should add some kind of test?

There are tests for the create/destroy aspects of the StatusBarManager API that would have detected the problem here: https://github.com/enthought/pyface/blob/9aeb3bd56bb6bec52b244132a928416e40d4b878/pyface/tests/test_application_window.py#L222-L252

Unfortunately those tests need the GuiTestAssistant class which we haven't yet integrated for WxPython (see #614 and #266 - the primary obstacle is not writing the class, it's getting everything running correctly once you have it).

So the long and short of it is, no, no need to write a test, and this looks good to go.

greschd commented 1 year ago

Thanks for the review, and the context on testing!