classification
Title: Stop using imp in IDLE
Type: behavior Stage: resolved
Components: IDLE Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, kbk, python-dev, roger.serwy, terry.reedy
Priority: normal Keywords: needs review, patch, pep3121

Created on 2013-05-25 15:40 by brett.cannon, last changed 2013-06-07 17:19 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
imp_removed_from_idle.diff brett.cannon, 2013-05-25 15:40 review
imp_remove_rev1.diff roger.serwy, 2013-05-26 00:06 review
Messages (8)
msg189968 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2013-06-07 17:19
Thanks for the help everyone!
History
Date User Action Args
2013-06-07 17:19:17brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg190761

stage: commit review -> resolved
2013-06-07 17:18:46python-devsetnosy: + python-dev
messages: + msg190760
2013-05-28 01:25:17brett.cannonsetassignee: brett.cannon
2013-05-26 03:54:49terry.reedysetmessages: + msg190059
2013-05-26 03:44:47terry.reedysetmessages: + msg190056
2013-05-26 00:06:41roger.serwysetfiles: + imp_remove_rev1.diff

messages: + msg190037
2013-05-25 22:54:23brett.cannonsetmessages: + msg190032
2013-05-25 21:58:07terry.reedysetkeywords: + needs review, pep3121

messages: + msg190021
versions: + Python 3.3
2013-05-25 21:36:38terry.reedylinkissue18064 dependencies
2013-05-25 15:50:49serhiy.storchakasetnosy: + terry.reedy, kbk, roger.serwy
2013-05-25 15:40:26brett.cannonlinkissue14797 dependencies
2013-05-25 15:40:14brett.cannoncreate