pytest-dev / pytest-qt

pytest plugin for Qt (PyQt5/PyQt6 and PySide2/PySide6) application testing
https://pytest-qt.readthedocs.io
MIT License
409 stars 70 forks source link

Simplify PEP8 method aliases in QtBot class. #383

Closed luziferius closed 3 years ago

luziferius commented 3 years ago

This commit simplifies the creation of PEP8 compliant method aliases.

It reduces call stack depth by one and enables better parameter suggestions in IDEs.

Before: qtbot_args

After: qtbot_args_new

Also added unit test verifying that each alias points to the correct source method.

(I hope that the test function location is OK)

nicoddemus commented 3 years ago

Thanks @luziferius for the PR!

However that way to declare the aliases was how it was done some time ago, until https://github.com/pytest-dev/pytest-qt/pull/326. Please take a look there to see the rationale and let us know what you think.

luziferius commented 3 years ago

Thanks @luziferius for the PR!

However that way to declare the aliases was how it was done some time ago, until #326. Please take a look there to see the rationale and let us know what you think.

Oops. Haven’t seen that. I just assumed it was a legacy implementation that was this way because of Python 2.x compatibility.

I’ll experiment with another solution then.

luziferius commented 3 years ago

I think I have found a solution.

Are you fine with a solution that breaks, if the subclass overwrites __init__() but does not perform a super() call in it’s own __init__() implementation?

I’d consider doing so an illegal use of the base class, so it should be fine from my point of view.

nicoddemus commented 3 years ago

Thanks!

I’d consider doing so an illegal use of the base class, so it should be fine from my point of view.

I agree, that would be fine.

luziferius commented 3 years ago

Done. The PEP 8 aliases are now only defined in __init__().

The unit tests added do:

nicoddemus commented 3 years ago

Thanks @luziferius!

The PR and the solution is very well written, and the added checks are the cherry on the cake. Thanks! 🍰

My very (small) concern is that we are creating a reference cycle in QtBot.__init__, but I don't think it will really be an issue given QtBot is small in size.

luziferius commented 3 years ago

My very (small) concern is that we are creating a reference cycle in QtBot.__init__, but I don't think it will really be an issue given QtBot is small in size.

Would you mind to elaborate on this?

Do you mean something like this below or something entirely different?

self.a = self.b
self.b = self.a
nicoddemus commented 3 years ago

I mean:

self.add_widget = self.addWidget

Creates a circular reference because self.add_widget is a bound method to self.addWidget, which references self.