Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade NVDA codebase to Python 3.7 #9543

Closed
wants to merge 59 commits into from

Conversation

josephsl
Copy link
Collaborator

@josephsl josephsl commented May 7, 2019

Hi,

Note: the branch is based on master, but it might be possible to rebase this on threshold. This is a different kind of pull request, in that I would like to invite everyone to participate - comments, commits, reviews, and so on.

Link to issue number:

Fixes #7105

Note: issues that are fixed by this pull request will be added as commits come in.

Summary of the issue:

Upgrades NVDA source code to Python 3, specifically, to Python 3.7.

Description of how this pull request fixes the issue:

Various issues were identified and fixed, including xrange vs. range, use of Python 3 module names for imports, dictionary iteration and key access (has_key vs key in dict) and others. Note that Python 3 work involves more than source code - see the linked issue for details.

Testing performed:

Source code level testing done since September 2018.

Known issues with pull request:

Dependencies will need to be edited and/or satisfied, some unforeseen errors. Note that these will be rectified as pull request writing progresses.

Change log entry:

Changes for developers:

And so many things to write.

Additional notes:

A while ago there was a theoretical pull request meant to test viability of Python 3 transition, which was based on a combination of Python 2 and 3 syntax, semantics, and assumptions. This is the real pull request that will use Python 3 native syntax, semantics, and assumptions. In short, I am donating the work done over the last few months to NV Access to help speed things up a bit.

Thanks.

@josephsl
Copy link
Collaborator Author

josephsl commented May 7, 2019

Hi,

Don't worry about failing tests - this is expected due to many differences. In order to resolve this, we need to move to Python 3 all the way - dependencies, scons, etc.

Thanks.

josephsl added 6 commits May 7, 2019 09:19
…langauge handler, MoveFileEx function in instlaler.

There are things that could see some optimizations:
* Language handler/pgettext: investigate if converting the string to Unicode is helpful.
* Installer: use MoveFileExW function directly if possible.
@LeonarddeR
Copy link
Collaborator

The conversion from basestring to str and unicode to str worries me a bit. Of course it is perfectly valid, however, we can no longer distinguish between code that used str initially and str after the conversion, other than looking at the code without these commits. I'd rather suggest to revert these commits, then look at every current use of str and change that into bytes if needed. Then, you can apply the conversion again.

@LeonarddeR
Copy link
Collaborator

Also, please change the base branch to threshold

@feerrenrut
Copy link
Contributor

Joseph, I haven't started looking at this code yet. But I imagine this process is uncovering lots of lessons on how this should be approached. I imagine the change set for the python 3 migration is going to be quite large, at least in terms of different areas of the code changed, if not the line count. This is going to make reviewing the code quite difficult. An alternative might be to review the process used (with explanations of why it should be done this way).

To be honest, I'd be quite hesitant to merge in any large PR that "muddies the waters" when it comes to the state of the transition.

@josephsl
Copy link
Collaborator Author

josephsl commented May 8, 2019 via email

@josephsl
Copy link
Collaborator Author

josephsl commented May 8, 2019

Hi,

Currently, I am doing the following:

  1. Test changes with another copy of the repo with Python 3 source code and dependencies applied.
  2. I'm actively avoiding speech module in case speech refactor introduces changes.
  3. I'm adding source code comments for some changes that appear to be unclear at first in aiding with review (for example, language handler and others).
  4. I'm grouping commits by syntax and semantic changes because they are scattered everywhere.

As for Leonard's request, I'll revert last night's commits on basestring/unicode/str/unichr/chr changes until we figure out how to proceed with this (see textUtils pull request for details).

Thanks.

josephsl added 12 commits May 25, 2019 09:36
.

Even in Python 2, open() function is used, and in Python 3, file() is gone. Thus unify under open() with explanatory comments added.
… statement changes in Python 3, applicable to log viewer and input core. Re nvaccess#7105.
…tion clal, cPickle, basestring, ugettext, encode/decode.

Follow-up from comment by Reef Turner (NV Access): add more explanatory comments, along iwth adding current and ideal replacements.
…tually no longer needed in Python 3. Re nvaccess#7105.

In order to deal with Unicode and non-Unicode paths, a dedicated function was provided to convert a package path to Unicode and return it. Since Python 3 can handle Unicode directly, consider removing this function altogether if no longer needed.
@josephsl
Copy link
Collaborator Author

Hi all,

I would like to request assistance from the community in documenting transition issues directly in source code (so far, I covered add-on handler, API module, app module, braille display detector, and main config module); it isn't enough for one person to do this, hence this request for participation from the entire community (and an invitation to people outside our project to provide guidance for us, along with helping us with this huge marathon).

For comment writers:

  1. Study the code carefully.
  2. Write down the comments in the following format:

Issue number (Py3 review required): short explanation.
Py2: current code
Py3: suggestion

This is so that NV Access people and others can think about differences and take appropriate action when it is time to review this pull request and related ones. Please pay careful attention to #8661 - basestring vs str, Unicode, encoding/decoding and others - and do suggest workarounds to @LeonarddeR.

For reviewers:

  1. As suggested by Reef, "Py3 review required" text has been added to comments for ease of review.
  2. I have added both Python 2 and 3 code fragments for ease of comparing differences and also to leave room for additional suggestions.

Thanks.

@michaelDCurran
Copy link
Member

This pr has been replaced by other smaller prs (some of which were cherry-picked from this pr). This pr has proven very useful in the conversion to Python3.

@josephsl
Copy link
Collaborator Author

josephsl commented Jun 12, 2019 via email

@josephsl josephsl deleted the i7105python3 branch September 16, 2019 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants