Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using imp in IDLE #62255

Closed
brettcannon opened this issue May 25, 2013 · 8 comments
Closed

Stop using imp in IDLE #62255

brettcannon opened this issue May 25, 2013 · 8 comments
Assignees
Labels
topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

BPO 18055
Nosy @brettcannon, @terryjreedy, @kbkaiser, @serwy
Files
  • imp_removed_from_idle.diff
  • imp_remove_rev1.diff
  • 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']

    @brettcannon
    Copy link
    Member Author

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

    @brettcannon brettcannon added topic-IDLE type-bug An unexpected behavior, bug, or error labels May 25, 2013
    @terryjreedy
    Copy link
    Member

    (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
    Copy link
    Member Author

    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
    Copy link
    Mannequin

    serwy mannequin commented 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:

    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.

    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Member

    Roger, sorry, I did not read your message completely. When I open from the console intepreter, I also get the traceback; same behavior.

    @brettcannon brettcannon self-assigned this May 28, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented 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
    Copy link
    Member Author

    Thanks for the help everyone!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants