Conversation
avalentino
left a comment
There was a problem hiding this comment.
Python 2.7 is also mentioned in a comment in qtpy/tests/test_qtpositioning.py.
6590ed4 to
b4821e7
Compare
CAM-Gerlach
left a comment
There was a problem hiding this comment.
Very thorough job, @dalthviz ! I ran a bunch of regexes I use to detect possible Py <3.6 blocks and code, as well as manually looked through both your changes and the codebase, and everything looked great. I also ran pyupgrade on the repo with pyupgrade --py36-plus setup.py qtpy/*.py qtpy/tests/*.py qtpy/_patch/*.py, which did spot and autofix a few remaining issues that allowed eliminating/simplifying a further >100 lines of code:
- Legacy
absolute_importfutureimports that were missed in the tests - Calls to the
mockthird-party backport instead of the built-inunittest.mock, which allowed eliminating it as an extra test dependency - Redundant
coding: utf-8declarations (since the default source file encoding on Python 3+ is UTF-8) - A few uses of legacy ordered
format()calls that could be safety rewritten as much simpler f-strings - A few other small odds and ends (a legacy
super()call, a legacy class declaration, and importing and usingio.open()instead of the now-identical and more concise/standard builtinopen()
The results of the run plus the accompanying manual tweaks (removing the now-unused import io and dropping mock from the test scripts) are on the fix_issue_250 branch on my fork. Assuming you agree with the changes, feel free to pull that and push here, or I can do so directly if that's easier.
|
Hi @CAM-Gerlach thank you for checking and futher helping with this! I will check the new changes and pull them here 👍 |
e5560f1 to
540d1be
Compare
|
I assume the CI issues are a result of rebasing on #225 , as I didn't see anything obviously connected to my changes. However, just in case you didn't already see it, in |
c43217d to
b0d28a1
Compare
CAM-Gerlach
left a comment
There was a problem hiding this comment.
LGTM @dalthviz ; thanks as always for all your hard work
Update RELEASE instructions too Remove py3compat module Remove sys.version based validations
b0d28a1 to
3900928
Compare
|
Hi @CAM-Gerlach , could you do a new review here to check if after the last rebase everything is still good? Thanks! Edit: Also, if is good go ahead and merge it 👍 |
|
Sure, in work 👍 |
There was a problem hiding this comment.
Thanks so much for all your work here, @dalthviz . I reviewed all the code changes, regexed for any remaining vestiges of Qt4/Python 2, and re-ran pyupgrade, and everything looks good aside from two issues. Since GitHub wouldn't let me make one of them as a suggestion, I just made them as separate commits (for ease of cherry-picking/dropping/fixuping etc) on my branch, like before, so if you like, you can easily just pull that in again and cherry-pick/squash/fixup/etc. to your liking if you agree with the changes, or if you decline the one I couldn't make as a suggestion, feel free to just apply the suggestion here.
Specifically, in addition to the comment about mock, not sure if it happened in the rebase, but most of lines 89-102 in _qfiledialog_wrapper() in compat.py appear to be dead code due to being only for Qt <=4.6, and can be replaced by just:
result = func(parent, caption, basedir, filters, selectedfilter, options)
if sys.platform == "win32":
# On Windows platforms: restore standard outputs
sys.stdout, sys.stderr = _temp1, _temp2(See a1380d7)
|
Oops, forgot to force push to the branch on my fork, sorry. Its up to date now; specifically |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
CAM-Gerlach
left a comment
There was a problem hiding this comment.
LGTM, thanks so much @dalthviz
Fixes #250