| msg189968 - (view) |
Author: Brett Cannon (brett.cannon) *  |
Date: 2013-05-25 15:40 |
| As part of issue #14797 I'm trying to remove all uses of imp.find_module/load_module since the API is so bad in the face of importers. See the attached patch to replace the one use in IDLE. It should actually be more accepting of alternative importers than the original code. Since I'm not an IDLE user I'm not comfortable with committing w/o an IDLE dev signing off on it, so please let me know if I can commit it (I would only bother in 3.4 unless you really want it in the 3.3 branch). |
|
|
| msg190021 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-05-25 21:58 |
| (Roger, please see Bretts opening message.) Brett, thanks for asking. We prefer to keep active codebases in sync as much as possible. Since new import stuff is not in 2.7, we will have to skip that. In EditorWindow.py, the patch deletes the _find_module function that wraps imp.find_module and edits the EditorWindows class open_module method that used that method. I presume that the new code somewhere duplicates the extra work done in _find_module beyond that done by imp.find_module. I verified that these are the only idlelib/*.py occurrences of 'find_module' and '_find_module'. It applies to 3.3 with chunk 3 offset a line. The only use of open_module is its binding to File / Open Module (Alt-M). This opens a module, given by its dotted import name, in the editor, instead of importing it. This works for stdlib modules before and after the patch. I tried both text selection and keyboard entry. As noted in the code, and explained in #18064, open_module does not work for 'current directory' modules. (A fix for that issue should wait for this one.) I did not try anything exotic like a relative path (never used one, not sure of syntax), .pth (not used on current machine), or package directories. But I expect the new import code is more reliable for these. If there is a problem, people can always use the open dialog. So unless Roger sees something I missed, you have my green light to apply. |
|
|
| msg190032 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2013-05-25 22:54 |
| If you would prefer to keep using imp in IDLE to make management across versions easier I'm fine with that, just understand my goal is to deprecate imp in Python 3.4 (but won't remove it until Python 4 so people can write Python 2/3 compatible code, much like you with IDLE). So if you're okay with using a deprecated module then this change is strictly not necessary and I can just silence any deprecation warning in the code instead when the deprecation comes about. |
|
|
| msg190037 - (view) |
Author: Roger Serwy (roger.serwy) * |
Date: 2013-05-26 00:06 |
| @Brett, I agree that IDLE should not be using deprecated modules. I don't know all the ins and outs of the import machinery of Python, so I'll defer to your expertise in that area. :-) @Terry, I have never used this IDLE feature before, so I don't know all it's corner cases or expected behavior. The patch works when opening "os" but fails with "sys". Without the patch, a dialog box appears saying "No source for module sys". With the patch, I get "not a source-based module" and then another dialog "loader does not support get_filename", and then this exception raises: Exception in Tkinter callback Traceback (most recent call last): File "/home/python/python/3.4/Lib/tkinter/__init__.py", line 1475, in __call__ return self.func(*args) File "/home/python/python/3.4/Lib/idlelib/EditorWindow.py", line 680, in open_module self.flist.open(file_path) UnboundLocalError: local variable 'file_path' referenced before assignment Serhiy's review comments on the patch about adding "return" in those three places does fix that problem. Attached is a revision to include Serhiy's suggestions. |
|
|
| msg190056 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-05-26 03:44 |
| Brett, you misunderstood somethings. I want to apply now to 3.3 and 3.4. This is the right time now that 3.2 final maintenance is out. (In any case, we are no longer patching it. 2.7 gets improvements on a 'reasonable best effort' basis.) It is also clearer and can be easily 'translated' into the skeleton of a testcase. Roger, ditto. But the intent is clear to me. On my Windows, trying to load 'sys' does bring up the 'source' message box, but without 'return', it next triggers the 'loader' message. A bad name gets all three boxes. Anyway, another example of why to test on more than one OS (assuming you did not use Windows). Another change: I think "not a source-based module" should be 'not a Python-coded module'. The intent of the feature is to open any python-coded module that can be imported. C-coded modules are also, ultimately, source-based. |
|
|
| msg190059 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-05-26 03:54 |
| Roger, sorry, I did not read your message completely. When I open from the console intepreter, I also get the traceback; same behavior. |
|
|
| msg190760 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-06-07 17:18 |
| New changeset a0d8ae880ae6 by Brett Cannon in branch '3.3': Issue #18055: Move to importlib from imp for IDLE. http://hg.python.org/cpython/rev/a0d8ae880ae6 New changeset 3a3ec484ce95 by Brett Cannon in branch 'default': merge w/ 3.3 for issue #18055 http://hg.python.org/cpython/rev/3a3ec484ce95 |
|
|
| msg190761 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2013-06-07 17:19 |
| Thanks for the help everyone! |
|
|