Python 3 imports: additional work on builtins, relative imports, importlib

[josephsl]

Link to issue number:

Fixes #8712
Fixes #8724
Fixes #8768

Summary of the issue:

More work on Python 3 imports, including relative imports and changing builtin to builtins, as well as using importlib to import modules at runtime.

Description of how this pull request fixes the issue:

Changes made:

• Import builtin -> builtins (Python Console, language handler).
• GUI and IAccessible objects: more relative imports (from . import ...).
• Use importlib to import modules at runtime, including for IAccessible modules.

Testing performed:

Tested with Python 2.7 interpreter to make sure they are imported.

Known issues with pull request:

None

Change log entry:

None

[LeonarddeR]

Could you call this with kwargs? Like:

mod=__import__(modString,globals=globals(),locals=locals(),level=1)

There is also the advice to use importlib.import_module, at least on python 2.

[josephsl]

Hi, importlib is new in Python 3 (if I read this correctly). Also, use of kwargs in __import__ is something we should consider for all instances of this, not just IAccessible. Thanks for pointing this out. From: Leonard de Ruijter <notifications@github.com> Sent: Friday, September 14, 2018 7:21 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports (#8727) @LeonarddeR commented on this pull request.

_____ In source/NVDAObjects/IAccessible/__init__.py <#8727 (comment)> :

@@ -416,7 +416,8 @@ def findOverlayClasses(self,clsList):

if classString and classString.find('.')>0: modString,classString=os.path.splitext(classString) classString=classString[1:] - mod=__import__(modString,globals(),locals(),[]) + # #8712: Python 3 wants a dot (.) when loading a module from the same folder via relative imports, and this is done via level argument. + mod=__import__(modString,globals(),locals(),[],1) Could you call this with kwargs? Like: mod=__import__(modString,globals=globals(),locals=locals(),level=1) There is also the advice to use importlib.import_module, at least on python 2. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8727 (review)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkAQSe69DIhXP-Ho8IZIejsIzft7vks5ua7thgaJpZM4WgJnV> .

[LeonarddeR]

@josephsl commented on 14 Sep 2018, 16:25 CEST:

Hi, importlib is new in Python 3 (if I read this correctly).

It is also there in Python 2.7.

Help on built-in function import in module builtin:

import(...)
import(name, globals={}, locals={}, fromlist=[], level=-1) -> module

Import a module. Because this function is meant for use by the Python

interpreter and not for general use, it is better to use
importlib.import_module() to programmatically import a module.

[josephsl]

Hi, I see. In this case, I propose doing this via a new pull request. Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Friday, September 14, 2018 12:11 PM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports (#8727) <https://github.com/josephsl> @josephsl commented on 14 Sep 2018, 16:25 CEST <#8727 (comment)> : Hi, importlib is new in Python 3 (if I read this correctly). It is also there in Python 2.7. Help on built-in function import in module builtin: import(...) import(name, globals={}, locals={}, fromlist=[], level=-1) -> module Import a module. Because this function is meant for use by the Python interpreter and not for general use, it is better to use importlib.import_module() to programmatically import a module. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8727 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkMnNPWG39Deuw8xp1sxP1qyVafteks5ua_81gaJpZM4WgJnV> .

[LeonarddeR]

I'd rather see this as part of this one. It creates unnecessary overhead if we have to change one line of code in this pr, and then change it again in the next.

See also #8768.

[josephsl]

Hi,

By the way, there is an unused "import Queue" in synthDrivers.espeak (no underscore), so I'm ignoring that one. People who will be working on transition should note that this import causes an exception, so should be removed then.

Thanks.

[LeonarddeR]

This is definitely going to fail on python 3

>>> "hello, %s" % "test".encode("mbcs")
"hello, b'test'"

str.format gives the same result.

It should be sufficient to check if not isinstance(appName, str) before encoding to mbcs.

[josephsl]

Hi, yep, it’ll definitely fail in Python 3. I’d reserve this for another pull request dealing with encoding/decoding work. Thanks for reminding me about this. From: Leonard de Ruijter <notifications@github.com> Sent: Monday, September 24, 2018 6:51 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports, importlib (#8727) @LeonarddeR requested changes on this pull request.

_____ In source/appModuleHandler.py <#8727 (comment)> :

@@ -93,8 +94,7 @@ def getAppNameFromProcessID(processID,includeExt=False):

# Try querying the app module for the name of the app being hosted. try: # Python 2.x can't properly handle unicode module names, so convert them. - mod = __import__("appModules.%s" % appName.encode("mbcs"), This is definitely going to fail on python 3

>> "hello, %s" % "test".encode("mbcs")

"hello, b'test'" str.format gives the same result. It should be sufficient to check if not isinstance(appName, str) before encoding to mbcs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8727 (review)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkAGndeV6pAGOkrg0PNVrAUdNNO7rks5ueOMxgaJpZM4WgJnV> .

[LeonarddeR]

I'm again not very happy with the idea to touch code in one pr, knowing that it requires another change in a subsequent pr. then, we either better leave the code alone in this pr or fix it for good. I'm happy with either way, but not with leaving as is, honestly.

[josephsl]

Hi, I’m willing to make an exception by addressing encoding problem as part of this PR with a comment stating that Python 3 improves Unicode support. Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Monday, September 24, 2018 7:57 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports, importlib (#8727) I'm again not very happy with the idea to touch code in one pr, knowing that it requires another change in a subsequent pr. then, we either better leave the code alone in this pr or fix it for good. I'm happy with either way, but not with leaving as is, honestly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8727 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkM0GjtdsQgRBupdEpav_TxDlS9Wiks5uePLVgaJpZM4WgJnV> .

[josephsl]

Hi,

Addressed the import concern as of today's commit. As noted in the commit message, I'm making an exception, as nobody else calling import_module calls name.encode apart from app module handler.

Thanks.

[LeonarddeR]

IMO its more elegant to do if isinstance(appName, bytes): bytes is a shortcut to str on python 2 and a separate type in python 3, so it will automatically apply to python 2 in this case. Alternatively, you can use sys.version_info.major

[josephsl]

Hi, in the short term, sys.version_info will work. Checking isinstance may provide a long-term solution, but it sends a message to others that bytes is a second class citizen (which is not, but it could be interpreted that way). Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Tuesday, September 25, 2018 12:21 PM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports, importlib (#8727) @LeonarddeR commented on this pull request.

_____ In source/appModuleHandler.py <#8727 (comment)> :

@@ -92,9 +92,12 @@ def getAppNameFromProcessID(processID,includeExt=False):

# This might be an executable which hosts multiple apps. # Try querying the app module for the name of the app being hosted. + # Python 2.x can't properly handle unicode module names, so convert them. + # No longer the case in Python 3. + if sys.version.startswith("2"): IMO its more elegant to do if isinstance(appName, bytes): bytes is a shortcut to str on python 2 and a separate type in python 3, so it will automatically apply to python 2 in this case. Alternatively, you can use sys.version_info.major — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8727 (review)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkNqj6KklCxD7Nys0io3GRmWaFzDLks5ueoIbgaJpZM4WgJnV> .

[LeonarddeR]

Never mind, I was horribly wrong there. This place actually encodes (goes from unicode from bytes), my head was telling me that we had to decode (from bytes to unicode). So if it would have been isinstance, the right clause would be if not isinstance(appname, str):

[josephsl]

Hi,

@feerrenrut, may I request a second look for this one? I'm interested in seeing if conflicts may arise when attempting to merge this pull request in the future.

Thanks.

[feerrenrut]

This looks good to me. Given that it is tested against python 2.7, I think we should merge it in now. We don't have any intention of continuing to support Python 2.7 once we move over to 3 so I will add a task to the python 3 project board to remove any 2.7 specific code.

[LeonarddeR]

I agree.

[josephsl]

Possible regression reported: multi-byte file name issue. Multiple possibilities, including Python 2.7.16, this PR, or something else.

Thanks.

[LeonarddeR]

@josephsl: Could you please file this again against threshold?

[josephsl]

Hi, yep that’ll be done (today at the earliest) with at least two separate PR’s: imports in one package, importlib in another. Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Tuesday, June 4, 2019 12:29 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] Python 3 imports: additional work on builtins, relative imports, importlib (#8727) @josephsl <https://github.com/josephsl> : Could you please file this again against threshold? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8727?email_source=notifications&email_token=AB4AXEE7F2SOX2QQ4RSJ7ETPYYKTJA5CNFSM4FUATHK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3VVSY#issuecomment-498555595> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEGNIJMDUODN4Z4IMVDPYYKTJANCNFSM4FUATHKQ> .