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

Remove functions in rope.base.ast that has functionally identical implementation in stdlib's ast #581

Merged
merged 8 commits into from Dec 9, 2022

Conversation

lieryan
Copy link
Member

@lieryan lieryan commented Dec 9, 2022

Description

  • Remove rope.base.ast.walk()
  • Remove rope.base.ast.get_children()
  • Remove rope.base.ast.get_child_nodes()

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md

This is a name clash with standard library's `ast.walk()`, which has a
different, and incompatible interface.
These functions are no longer be necessary in supported versions of
Python, as a version with equivalent functionalities are now included in
stdlib `ast`.
This has been replaced with iter_child_nodes()

Note that iter_child_nodes() returns an iterator while get_child_nodes()
returns a List.
This is now replaced with ast.iter_fields()

Note that iter_fields() returns an iterator while get_children()
returns a List.
This is functionally identical to stdlib's ast.NodeVisitor
@lieryan lieryan self-assigned this Dec 9, 2022
@lieryan
Copy link
Member Author

lieryan commented Dec 9, 2022

FYI, @edreamleo to simplify the discussion a bit; let's start by trying to remove unnecessary parts of rope.base.ast

These are legacy implementations which I suppose likely was written back when stdlib's ast didn't have these functionalities.

There's no need for rope to maintain these in rope.base.ast

@lieryan lieryan changed the title Remove code in rope.base.ast that has functionally identical implementation in stdlib's ast Remove functions in rope.base.ast that has functionally identical implementation in stdlib's ast Dec 9, 2022
@lieryan lieryan merged commit c0bec17 into master Dec 9, 2022
@lieryan lieryan deleted the lieryan-rope-base-ast-2 branch December 9, 2022 17:14
@edreamleo
Copy link
Contributor

@lieryan Good idea.

@edreamleo
Copy link
Contributor

edreamleo commented Dec 9, 2022

I just pulled master and did a trial merge into ekr-reorg3, using git checkout --theirs to resolve conflicts. Quite a few unit tests fail:

AttributeError: module 'rope.base.astutils' has no attribute 'call_for_nodes'

I'll investigate. At worst I'll just abort the merge :-)

@edreamleo
Copy link
Contributor

The crash occurs because of this line in _ResultChecker._find_node in patchedasttest.py:

astutils.call_for_nodes(self.ast, search, recursive=True)

I'm confused: I don't see patchedasttest.py in the list of changed files for this PR.

Anyway, all unit tests pass after making these changes to patchedasttest.py:

  • from rope.base.ast import call_for_nodes
  • In _find_node, call call_for_nodes(self.ast, search, recursive=True)

Do you have any idea what happened?

@edreamleo
Copy link
Contributor

@lieryan I like what I see at first glance. Thanks for meeting me halfway.

@edreamleo
Copy link
Contributor

Another mystery. In my original merge into ekr-reorg3 rope.base.ast still contained a parse function. With that parse function in place all tests pass.

One unit tests fails after removing (as I think I should) the parse function from rope.base.ast:

====================================================== FAILURES =======================================================
____________________________________ PyCoreTest.test_syntax_errors_when_null_bytes ____________________________________

self = <ropetest.pycoretest.PyCoreTest testMethod=test_syntax_errors_when_null_bytes>

    def test_syntax_errors_when_null_bytes(self):
        mod = testutils.create_module(self.project, "mod")
        contents = b"\n\x00\n"
        file = open(mod.real_path, "wb")
        file.write(contents)
        file.close()
        with self.assertRaises(exceptions.ModuleSyntaxError):
>           self.pycore.resource_to_pyobject(mod)

ropetest\pycoretest.py:734:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
rope\base\pycore.py:141: in resource_to_pyobject
    return self.module_cache.get_pymodule(resource, force_errors)
rope\base\pycore.py:262: in get_pymodule
    result = pyobjectsdef.PyModule(
rope\base\pyobjectsdef.py:173: in __init__
    source, node = self._init_source(pycore, source, resource)
rope\base\pyobjectsdef.py:200: in _init_source
    ast_node = ast.parse(source_bytes, filename=filename)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    def parse(source, filename='<unknown>', mode='exec', *,
              type_comments=False, feature_version=None):
        """
        Parse the source into an AST node.
        Equivalent to compile(source, filename, mode, PyCF_ONLY_AST).
        Pass type_comments=True to get back type comments where the syntax allows.
        """
        flags = PyCF_ONLY_AST
        if type_comments:
            flags |= PyCF_TYPE_COMMENTS
        if isinstance(feature_version, tuple):
            major, minor = feature_version  # Should be a 2-tuple.
            assert major == 3
            feature_version = minor
        elif feature_version is None:
            feature_version = -1
        # Else it should be an int giving the minor version for 3.x.
>       return compile(source, filename, mode, flags,
                       _feature_version=feature_version)
E       ValueError: source code string cannot contain null bytes

C:\Python\Python3.10\lib\ast.py:50: ValueError
=============================================== short test summary info ===============================================
FAILED ropetest/pycoretest.py::PyCoreTest::test_syntax_errors_when_null_bytes - ValueError: source code string cannot...
=============================== 1 failed, 1938 passed, 21 skipped, 5 xfailed in 25.60s ================================

@edreamleo
Copy link
Contributor

@lieryan Fixed at 4242dc9. It was probably just a "normal merge error".

ekr-reorg3 contains calls to ast.parse and rope.base.parse. I haven't yet tried to make sense of it all. There still seems to be considerable confusion.

@lieryan lieryan added this to the 1.6.0 milestone Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants