DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.24k stars 40 forks source link

Refactory of the code base #145

Closed DomHeadroom closed 1 year ago

DavidoTek commented 1 year ago

Great! I planned to refactor the code for a while now, but I haven't gotten to it yet. Looks good so far. I will take a closer look in the next days and see if I can merge.

Thanks! :tada:

sonic2kk commented 1 year ago

(just a nosy user passing by 😅)

Just wondering, since a lot of effort is going into this, should there be any changes to the * wildcard imports? Not sure how much of a difference it would make but since changes are being made here anyway, is it worth it to refactor those as well?

Also, just for my own curiosity, what's the benefit of using list() over []?

DomHeadroom commented 1 year ago

Not noisy at all, it's a pleasure for me to respond to the various request.

Are you talking about the fact that pupgui2.py is not in the root of the repo anymore, and some of the imports are no longer in a sub directory?

List() is sometimes faster than a list comprehension, otherwise the two have equal execution times. And in the various parts, that I changed them, resulted in less code and cleaner code.

sonic2kk commented 1 year ago

I didn't know it could be faster. That's really useful to know!

For the imports, I meant replacing things like from PySide6.blah import * with more explicit importing, like from PySide6.blah import aaa, bbb, ccc

Don't get me wrong, I understand it would be a lot of work, and there may not be much benefit if any benefit at all. I'm just asking because there's been a lot of refactoring here already, I'm wondering if it's worth making this change also and if it could bring any benefits? 😃

DavidoTek commented 1 year ago

For the imports, I meant replacing things like from PySide6.blah import * with more explicit importing, like from PySide6.blah import aaa, bbb, ccc [...] because there's been a lot of refactoring here already

Yeah, I think cleaning the imports up a bit is not a bad idea. It will delay the merge a bit, but I guess we can add that to this PR. Removing the wildcards can be good for several reasons. Also, we can clean up the other import statements (in some file there is import sys, os and in other there are two separate lines for that).

Are you talking about the fact that pupgui2.py is not in the root of the repo anymore, and some of the imports are no longer in a sub directory?

We could change imports from from .util import apply_dark_theme to from pupgui2.util import apply_dark_theme and from ...util import host_which to from pupgui2.util import host_which. Is that what you mean?

List() is sometimes faster than a list comprehension, otherwise the two have equal execution times.

That's interesting to know

PS: I reviewed the code, looks good, thanks :+1: