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

pyclbr rewrite using AST #83592

Closed
BatuhanTaskaya mannequin opened this issue Jan 21, 2020 · 5 comments
Closed

pyclbr rewrite using AST #83592

BatuhanTaskaya mannequin opened this issue Jan 21, 2020 · 5 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@BatuhanTaskaya
Copy link
Mannequin

BatuhanTaskaya mannequin commented Jan 21, 2020

BPO 39411
Nosy @terryjreedy, @brandtbucher, @isidentical
PRs
  • bpo-39411: pyclbr rewrite on AST #18103
  • 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 = None
    closed_at = <Date 2020-11-14.21:47:11.175>
    created_at = <Date 2020-01-21.13:29:29.292>
    labels = ['type-feature', 'library', '3.9']
    title = 'pyclbr rewrite using AST'
    updated_at = <Date 2020-11-14.21:47:11.174>
    user = 'https://bugs.python.org/BatuhanTaskaya'

    bugs.python.org fields:

    activity = <Date 2020-11-14.21:47:11.174>
    actor = 'BTaskaya'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-11-14.21:47:11.175>
    closer = 'BTaskaya'
    components = ['Library (Lib)']
    creation = <Date 2020-01-21.13:29:29.292>
    creator = 'Batuhan Taskaya'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39411
    keywords = ['patch']
    message_count = 5.0
    messages = ['360397', '360440', '360458', '360506', '380737']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'brandtbucher', 'BTaskaya', 'Batuhan Taskaya']
    pr_nums = ['18103']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue39411'
    versions = ['Python 3.9']

    @BatuhanTaskaya
    Copy link
    Mannequin Author

    BatuhanTaskaya mannequin commented Jan 21, 2020

    pyclbr currently uses token streams to analyze but it can be alot simpler with usage of AST. There are already many flaws, including some comments about limitations of this token stream processing.

    I have a draft about this. Initial PR wont change any behavior, it will just make code much simpler with the usage of AST (just an addition to Function about handling of async functions, is_async field). If agreed I can propose a second PR (or append the inital one) that will enhance Function/Class objects with various identifiers (like keywords, metaclasses, end position information etc.). The second PR will be alot easier to do thanks to AST.

    @BatuhanTaskaya BatuhanTaskaya mannequin added 3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jan 21, 2020
    @terryjreedy
    Copy link
    Member

    Nice. A few notes.

    Docs: The initial doc paragraph now has 'module browser' rather than 'class browser'. The change is needed 2 other places. Could be part of this PR or another one.

    • Chapter title: "Python class browser support". The only instance of 'class browser' in the chapter.
    • Module index: "Supports information extraction for a Python class browser."

    Possible API changes based on IDLE module browser experience:

    1. IDLE filters out the import nodes. It would be nicer to not have to delete them by passing something like 'imports=False' (with True being the default for back compatibility).

    2. With nested classes and functions added, the returned object is almost a tree of nodes, except that what should be the top 'Module' node is instead what should be its children dict. This un-uniformity gets in the way of recursively processing the tree.

    3. If there were a function that returned a Module node with its path and name, the same could be removed from all children except for new ImpClass nodes representing imported classes when imports=True. Since users know the module passed to pyclbr, this might be true even without Module nodes.

    4. If a user immediately inserts nodes into a tree widget, then the user may only need a iterator of minimal nodes (name, parent, startline). This is enough when using tkinter.ttk.Treeview. I have thought of doing this for IDLE by either simplifying a copy of the pyclbr code or traversing the AST, as the PR here does. This would let top-level items appear in the widget as they are scanned instead of waiting until scanning is complete.

    @terryjreedy terryjreedy changed the title pyclbr rewrite on AST pyclbr rewrite using AST Jan 22, 2020
    @terryjreedy terryjreedy changed the title pyclbr rewrite on AST pyclbr rewrite using AST Jan 22, 2020
    @isidentical
    Copy link
    Member

    Thanks for the suggestions Terry, I didn't want to change any behavior or make any feature addition (except is_async as stated in issue description) in this PR.

    The change is needed 2 other places. Could be part of this PR or another one.

    I have a friend who would like to make his first contribution, so if it is not going to be problem, he'll take care of class browser references.

    Possible API changes based on IDLE module browser experience

    Yes, it would be way better use tree/node structre instead of returning raw dictionaries. I can draft something after PR 1803 is merged.

    I dont have much experience of IDLE's architecture, but I can try to help if you want on the 4th point.

    Another thing I was considering is deprecating readmodule function, as you stated it is now module browser not class browser and readmodule_ex is sufficient. I dont know if any core developer will sponsor this idea, but I think readmodule should die. Thanks again for the suggestions and PR reviews.

    @terryjreedy
    Copy link
    Member

    A separate doc change issue and PR would be fine. Should we add a note explaining the module name as a contraction of an originally restricted scope? Make me nosy and invite review.

    Actually, a doc issue for the module as is should *fully* explain readmodule_ex first (its entry is now incomplete), and then explain readmodule as a filtered version kept for back compatibility. This could be a separate PR on the same issue, written by one of us, if too much for your friend.

    I understood limits of this PR. I should have said change notes were intended for your 'second PR'.

    Changing the return value to a Module should mean a third function, which would then become the main function, as readmodule_ex would then be Module.children.

    I have thought about making it possible to browse non-source modules, at least for the module being browsed. I might implement that first in IDLE.

    @isidentical
    Copy link
    Member

    New changeset fa476fe by Batuhan Taskaya in branch 'master':
    bpo-39411: pyclbr rewrite on AST (bpo-18103)
    fa476fe

    @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
    3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants