Skip to content

Upgrade Python syntax with pyupgrade#10405

Closed
AAClause wants to merge 2 commits into
nvaccess:masterfrom
AAClause:otherPy3Upgrade
Closed

Upgrade Python syntax with pyupgrade#10405
AAClause wants to merge 2 commits into
nvaccess:masterfrom
AAClause:otherPy3Upgrade

Conversation

@AAClause

@AAClause AAClause commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

pyupgrade is a tool to automatically upgrade syntax for newer versions of the language. Various improvements are suggested on NVDA's source code.

Description of how this pull request fixes the issue:

Moved to 'source' folder then executed pyupgrade --py36-plus --keep-percent-format on *.py files (recursively). Due to translatable strings, --keep-percent-format argument is required. See #10384.

Testing performed:

Tested that NVDA starts properly with these changes (from sources and a non-archived binary build).

Known issues with pull request:

None

Change log entry:

None

@josephsl

josephsl commented Oct 20, 2019 via email

Copy link
Copy Markdown
Contributor

@AppVeyorBot

AppVeyorBot commented Oct 20, 2019

Copy link
Copy Markdown

PR introduces Flake8 errors 😲

See test results for Failed build of commit 637bf8c88f

@Brian1Gaff

Brian1Gaff commented Oct 20, 2019 via email

Copy link
Copy Markdown

@LeonarddeR

LeonarddeR commented Oct 21, 2019

Copy link
Copy Markdown
Collaborator

While I like the changes this tool proposes, I agree with @josephsl that in the current stage of development, it might be better not to apply them now. I also think that we're still required to check them all by hand, just to make sure. Not to mention the huge amount of flake8 warnings.

@AAClause

AAClause commented Oct 21, 2019

Copy link
Copy Markdown
Contributor Author

I understand your concerns. But, in my opinion, we must dare these changes.
Indeed, the changes that this tool suggests seem relatively safe and are documented. Only 1% of the source code is concerned by these changes. It's widely reviewable.

I also think that we're still required to check them all by hand, just to make sure.

Of course! It is necessary to review all changes. Although there are many flake8 warnings, this is easily corrected. It's just time consuming. I'm willing to do that.

@AppVeyorBot

AppVeyorBot commented Oct 21, 2019

Copy link
Copy Markdown

PR introduces Flake8 errors 😲

See test results for Failed build of commit fd84da6c58

@LeonarddeR

LeonarddeR commented Oct 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

I"m tempted to close this as per several reasons:

  1. I think this pr proposes a very big change to the source code. Therefore, it makes sense to discuss the proper strategy in an issue first, before opening a pr.
  2. It's not clear to me why you're using --py36-plus and not --py37-plus. We are at version 3.7. IN fact, it might even make sense to delay this until we are at Python 3.8 and there is a --py38-plus flag. But that's why I propose to have a design table issue first.
  3. This pr removes the # -*- coding: UTF-8 -*- comments. They still seem to be required by xgettext, though. Therefore, tests are failing.

@AAClause

AAClause commented Oct 23, 2019

Copy link
Copy Markdown
Contributor Author

It's not clear to me why you're using --py36-plus and not --py37-plus.

Totally right. However, it's not a problem. The PR is in Draft status. I can wait several weeks, rebase my branch onto master and reapply the process without difficulty.

This pr removes the # -*- coding: UTF-8 -*- comments. They still seem to be required by xgettext, though. Therefore, tests are failing.

It's also not a problem. My 2nd commit (f456cd4) fixes this. To solves this, xgettext should be called using --from-code=UTF-8 by editing sconstruct file. Perhaps that another change is also required but it's a detail.

Currently the Flake8 warnings are the only problem for me. But as already said, that's easily corrected.

For info, the try build is here. No problem found until now with my tests.

@dpy013

dpy013 commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

It's not clear to me why you're using --py36-plus and not --py37-plus.

Totally right. However, it's not a problem. The PR is in Draft status. I can wait several weeks, rebase my branch onto master and reapply the process without difficulty.

This pr removes the # -*- coding: UTF-8 -*- comments. They still seem to be required by xgettext, though. Therefore, tests are failing.

It's also not a problem. My 2nd commit (f456cd4) fixes this. To solves this, xgettext should be called using --from-code=UTF-8 by editing sconstruct file. Perhaps that another change is also required but it's a detail.

Currently the Flake8 warnings are the only problem for me. But as already said, that's easily corrected.

For info, the try build is here. No problem found until now with my tests.

hi Andre
Can upgrade the current pr python to python3.7.5
thanks

@AAClause

AAClause commented Oct 23, 2019

Copy link
Copy Markdown
Contributor Author

@dingpengyu I've just run the --py37-plus with PyUpgrade. Result: there is no other change made.

By the way, the PyUpgrade documentation says:

--py37-plus will also remove generator_stop

Apparently it's the only difference with --py36-plus.

@dpy013

dpy013 commented Oct 24, 2019

Copy link
Copy Markdown
Contributor

@dingpengyu I've just run the --py37-plus with PyUpgrade. Result: there is no other change made.

By the way, the PyUpgrade documentation says:

--py37-plus will also remove generator_stop

Apparently it's the only difference with --py36-plus.

hi Andre
Thank you for your instructions
I still hope that the current pr version of pr can be consistent with the python version of the current source code.
thanks

@lukaszgo1

lukaszgo1 commented Oct 26, 2019

Copy link
Copy Markdown
Contributor

My personal opinion is that performing these changes all at once makes no sense. This creates very big diff which someone has to review and it is not good use of anyone's time given the fact that there are no immediate benefits.
Having source code inline with syntacs of current version of Python is however important What about doing it similar to linting process i.e. during PR building on AppVeyor execute PyUpgrade on files that are modified as part of PR in question?
It would achieve our goal and the changes would be incremental.

@LeonarddeR

LeonarddeR commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

@Andre9642 Could you please file a separate pr with the fix fro mhttps://github.com/nvaccess/nvda/commit/f456cd4afa5c65b909c3405daedae91817c3771b regarding encoding at the top of the file?

I stil ltend to agree with @lukaszgo1 in #10405 (comment) . I'm closing this pull request for now. Please open a separate issue to discuss the best approach for syntax upgrades.

@LeonarddeR LeonarddeR closed this Nov 6, 2019
@lukaszgo1 lukaszgo1 mentioned this pull request Dec 5, 2020
Merged
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@AAClause @josephsl @AppVeyorBot @Brian1Gaff @LeonarddeR @dpy013 @lukaszgo1