move patched methods to parent class

[comhar]

#609

This PR updates py2pyi so that patched methods are moved to their parent class. See #609 for more info.

TODO

• verify that the general approach is reasonable
• add unit tests
• add multi-parent support

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[comhar]

@jph00 I've taken a quick stab at #609.

If you think the general approach is ok, I'll go ahead and add:

• support for nested classes (assuming we need to support this case)
• unit tests.

I've been live testing using the example below.

Example

from fastcore.utils import patch


class A:
    def a(self, b, c):
        pass


class B:
    def b(self):
        pass


class C:
    def c(self):
        pass

    def c2(self):
        pass


@patch
def patch_a(self: A):
    pass


@patch
def a(self: A, x, y, z):
    pass


def unpatched_method():
    pass


@patch
def patch_a2(self: A):
    pass


@patch
def patch_c(self: C):
    pass


def unpatched_method2():
    pass

Here's the result.

Result

from fastcore.utils import patch

class A:

    def a(self, x, y, z):
        ...

    def patch_a(self):
        ...

    def patch_a2(self):
        ...

class B:

    def b(self):
        ...

class C:

    def c(self):
        ...

    def patch_c(self):
        ...

def some_f():
    ...

def some_f2():
    ...

[jph00]

Well done -- very impressive! :D On nested classes -- I think it's fine to skip that, personally, unless it's easy to add with minimal complexity.

[comhar]

Well done -- very impressive! :D On nested classes -- I think it's fine to skip that, personally, unless it's easy to add with minimal complexity.

Thanks 😊. I've added unit tests and multi-parent support. I haven't explored nested classes. Might be better to handle that scenario in a separate issue (if at all) and keep this PR as concise as possible.

Note: I only added a single unit test for _proc_patches as the underlying methods in _proc_patches have good coverage and we should be able to catch future regressions by inspecting any changes to test_py2pyi.pyi.

[comhar]

Both imports are now unused. Thoughts on creating a separate issue to remove them?

[jph00]

Sure feel free.

[comhar]

As fastcore doesn't have any dependencies that automatically remove unused imports (correct me if I'm wrong) I took a quick look at rolling our own unused import remover (just for delegates and patches). While the code snippet below works, it is ~30 lines long and doesn't cover all import situations. No doubt there are cleaner solutions out there but given the small benefit of removing the imports, I guess it is probably better to leave things as is and not implement the import remover?

Unused Import Remover

def _remove_unused_imports(tree):
    "Remove patch/delegates imports if unused."
    should_remove_delegates_import = True
    should_remove_patch_import = True

    # let's check if the delegates and patch decorators are used
    for node in ast.walk(tree):
        if not hasattr(node, "decorator_list"):
            continue
        for deco in node.decorator_list:
            deco_id = _deco_id(deco)
            if deco_id == "delegates": should_remove_delegates_import = False
            elif deco_id == "patch": should_remove_patch_import = False
            # if both decorators are being used we can exit early
            if should_remove_delegates_import is False and should_remove_patch_import is False:
                return

    # let's remove the delegates and patch imports if they exist and are unused
    i = 0
    while i < len(tree.body):
        node = tree.body[i]
        # TODO: Are there other ways to import delegates and patch?
        if isinstance(node, ast.ImportFrom):
            for imp in node.names:
                if imp.name == "delegates" and should_remove_delegates_import:
                    tree.body.pop(i)
                    i -= 1
                    break
                elif imp.name == "patch" and should_remove_patch_import:
                    tree.body.pop(i)
                    i -= 1
                    break
        i += 1

[jph00]

No I don't think so - there's no harm having unused imports, and you can't always know for sure if something is used dynamically.

…

Message ID: ***@***.***>