Repository navigation
Stop using imp in IDLE #62255
Description
| BPO | 18055 |
|---|---|
| Nosy | @brettcannon, @terryjreedy, @kbkaiser, @serwy |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/brettcannon'
closed_at = <Date 2013-06-07.17:19:17.086>
created_at = <Date 2013-05-25.15:40:14.456>
labels = ['expert-IDLE', 'type-bug']
title = 'Stop using imp in IDLE'
updated_at = <Date 2013-06-07.17:19:17.085>
user = 'https://github.com/brettcannon'bugs.python.org fields:
activity = <Date 2013-06-07.17:19:17.085>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2013-06-07.17:19:17.086>
closer = 'brett.cannon'
components = ['IDLE']
creation = <Date 2013-05-25.15:40:14.456>
creator = 'brett.cannon'
dependencies = []
files = ['30365', '30376']
hgrepos = []
issue_num = 18055
keywords = ['patch', 'needs review', 'pep3121']
message_count = 8.0
messages = ['189968', '190021', '190032', '190037', '190056', '190059', '190760', '190761']
nosy_count = 5.0
nosy_names = ['brett.cannon', 'terry.reedy', 'kbk', 'roger.serwy', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18055'
versions = ['Python 3.3', 'Python 3.4']
Activity
brettcannon commentedon May 25, 2013
As part of issue bpo-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).
terryjreedy commentedon May 25, 2013
(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 bpo-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.
brettcannon commentedon May 25, 2013
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.
serwy commentedon May 26, 2013
@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:
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.
terryjreedy commentedon May 26, 2013
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.
terryjreedy commentedon May 26, 2013
Roger, sorry, I did not read your message completely. When I open from the console intepreter, I also get the traceback; same behavior.
python-dev commentedon Jun 7, 2013
New changeset a0d8ae880ae6 by Brett Cannon in branch '3.3':
Issue bpo-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 bpo-18055
http://hg.python.org/cpython/rev/3a3ec484ce95
brettcannon commentedon Jun 7, 2013
Thanks for the help everyone!