Python 3 imports: additional work on builtins, relative imports, importlib by josephsl · Pull Request #8727 · nvaccess/nvda

josephsl · 2018-09-09T05:19:54Z

Link to issue number:

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

* IAccessible: from winWord import ... -> from .winWord import ... * GUI: from addonGui import ... -> from .addonGui import ...

Python Console and language handler imports __builtin__, which has been renamed to builtins in Python 3.

…ccessible support modules from the same folder. Re nvaccess#8712. Python 3 wants a dot (.) when importing modules from the same folder. When using __import__ function, the level argument specifies search level (default is 0). Thus allow other IAccessible modules to be loaded by specifying level of 1 (relative import from the same folder).

LeonarddeR · 2018-09-14T14:20:12Z

 				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.

josephsl · 2018-09-14T14:25:28Z

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 · 2018-09-14T19:10:43Z

@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 · 2018-09-14T20:36:50Z

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 · 2018-09-20T05:10:14Z

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.

…ly at runtime. Re nvaccess#8768. Python 2.7 includes a base implementation of importlib which powers __import__ function, with Python 3 providing more features such as importers. Thus use importlib.import_module in app module handler and others, specifying appropriate package names in the process.

nvaccess#8712. Because importlib.import_module complains about module names, spell the module name (NVDAObjects.IAccessible.mod).

josephsl · 2018-09-20T06:18:45Z

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.

HRESULT no longer exists in ctypes.wintypes in Python 3. The new location is ctypes.HRESULT. Even in Python 2, ctypes.wintypes.HRESULT points to ctypes.HRESULT.

LeonarddeR · 2018-09-24T13:49:50Z

 	# 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.

josephsl · 2018-09-24T14:25:56Z

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 · 2018-09-24T14:56:32Z

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 · 2018-09-24T15:00:41Z

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> .

…thon 2, no longer needed in Python 3. Addressing review comment (reviewed by Leonard de Ruijter/Babbage): appName.encode('mbcs') will fail in Python 3, so do not do it unless one is using Python 2. Note that this is an exception to the PR stream: nowhere in import_module call does encoding takes place.

josephsl · 2018-09-25T19:14:23Z

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 · 2018-09-25T19:20:57Z

 	# 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

josephsl · 2018-09-25T19:23:13Z

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 · 2018-09-25T19:30:08Z

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 · 2018-12-07T19:14:46Z

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 · 2019-05-02T14:19:18Z

I agree.

josephsl · 2019-05-29T06:08:01Z

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

Thanks.

…ts, importlib (PR #8727)" This reverts commit 8cbd3bb.

…ts, importlib (PR #8727)" (#9657) This reverts commit 8cbd3bb.

LeonarddeR · 2019-06-04T07:28:48Z

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

josephsl · 2019-06-04T10:13:58Z

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> .

josephsl added 4 commits September 6, 2018 23:54

Python 3/imports: additional relative imports (from . import module).

16f55ee

Python 3: more from . import module edits.

62aefc7

* IAccessible: from winWord import ... -> from .winWord import ... * GUI: from addonGui import ... -> from .addonGui import ...

Python 3 import: __builtin__ -> builtins. Re nvaccess#8724.

462b43d

Python Console and language handler imports __builtin__, which has been renamed to builtins in Python 3.

josephsl requested a review from feerrenrut September 9, 2018 05:20

josephsl added the z Python 3 transition (archived) Python 3 transition label Sep 9, 2018

LeonarddeR reviewed Sep 14, 2018

View reviewed changes

Merge branch 'master' into py3imports2

4e9d1f1

josephsl changed the title ~~Python 3 imports: additional work on builtins, relative imports~~ Python 3 imports: additional work on builtins, relative imports, importlib Sep 20, 2018

josephsl added 2 commits September 19, 2018 23:11

IAccessible: use importlib.import_module to load appropriate module. Re

2c4b96f

nvaccess#8712. Because importlib.import_module complains about module names, spell the module name (NVDAObjects.IAccessible.mod).

josephsl added 2 commits September 20, 2018 06:32

App module handler/imports: typo.

50d0688

Python 3/ctypes: HRESULT no longer exists in ctypes.wintypes.

e44368b

HRESULT no longer exists in ctypes.wintypes in Python 3. The new location is ctypes.HRESULT. Even in Python 2, ctypes.wintypes.HRESULT points to ctypes.HRESULT.

LeonarddeR suggested changes Sep 24, 2018

View reviewed changes

josephsl added 2 commits September 25, 2018 11:49

Merge branch 'master' into py3imports2

28f1068

LeonarddeR reviewed Sep 25, 2018

View reviewed changes

Python 3/app module handler: use sys.version_info.major.

29cb8b4

LeonarddeR approved these changes Sep 25, 2018

View reviewed changes

feerrenrut approved these changes May 2, 2019

View reviewed changes

feerrenrut merged commit 8cbd3bb into nvaccess:master May 2, 2019

nvaccessAuto added this to the 2019.2 milestone May 2, 2019

nishimotz mentioned this pull request May 20, 2019

Application with multibyte file name raises error #9583

Closed

feerrenrut mentioned this pull request May 28, 2019

Remove Python 2 imports for modules renamed in python 3 #8677

Closed

nishimotz mentioned this pull request May 29, 2019

fixes regression regarding multibyte filename executables #9635

Closed

josephsl added a commit that referenced this pull request May 31, 2019

Revert "Python 3 imports: additional work on builtins, relative impor…

b4d32c1

…ts, importlib (PR #8727)" This reverts commit 8cbd3bb.

josephsl mentioned this pull request May 31, 2019

Revert #8727 - built-ins, relative imports, importlib #9657

Merged

josephsl added a commit that referenced this pull request May 31, 2019

Revert "Python 3 imports: additional work on builtins, relative impor…

5252322

…ts, importlib (PR #8727)" This reverts commit 8cbd3bb.

josephsl added a commit that referenced this pull request May 31, 2019

Revert "Python 3 imports: additional work on builtins, relative impor…

6269143

…ts, importlib (PR #8727)" This reverts commit 8cbd3bb.

michaelDCurran pushed a commit that referenced this pull request Jun 1, 2019

Revert "Python 3 imports: additional work on builtins, relative impor…

ef0c98a

…ts, importlib (PR #8727)" (#9657) This reverts commit 8cbd3bb.

josephsl deleted the py3imports2 branch July 19, 2020 22:14

Uh oh!

Conversation

josephsl commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

josephsl commented Sep 14, 2018 via email

Uh oh!

LeonarddeR commented Sep 14, 2018

Uh oh!

josephsl commented Sep 14, 2018 via email

Uh oh!

LeonarddeR commented Sep 20, 2018

Uh oh!

josephsl commented Sep 20, 2018

Uh oh!

LeonarddeR Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

josephsl commented Sep 24, 2018 via email

Uh oh!

LeonarddeR commented Sep 24, 2018 via email

Uh oh!

josephsl commented Sep 24, 2018 via email

Uh oh!

josephsl commented Sep 25, 2018

Uh oh!

LeonarddeR Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

josephsl commented Sep 25, 2018 via email

Uh oh!

LeonarddeR commented Sep 25, 2018

Uh oh!

josephsl commented Dec 7, 2018

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented May 2, 2019

Uh oh!

josephsl commented May 29, 2019

Uh oh!

LeonarddeR commented Jun 4, 2019

Uh oh!

josephsl commented Jun 4, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

josephsl commented Sep 9, 2018 •

edited

Loading