jsonargparse icon indicating copy to clipboard operation
jsonargparse copied to clipboard

Fix/callable protocols 2

Open MiguelMonteiro opened this issue 1 year ago • 3 comments

What does this PR do?

Fixes callable protocol inheritance by allowing a list of private methods to be considered when matching signatures for inheritance of subclasses. The set of allowed_dunder_methods currently only contains the __call__ method.

Fixes #595

Before submitting

  • [x] Did you read the contributing guideline?
  • [ ] Did you update the documentation? (readme and public docstrings)
  • [ ] Did you write unit tests such that there is 100% coverage on related code? (required for bug fixes and new features)
  • [x] Did you verify that new and existing tests pass locally?
  • [x] Did you make sure that all changes preserve backward compatibility?
  • [x] Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

MiguelMonteiro avatar Oct 15 '24 12:10 MiguelMonteiro

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (442f9bf) to head (bd03440). Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #599   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         6491      6493    +2     
=========================================
+ Hits          6491      6493    +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 15 '24 12:10 codecov[bot]

Please add a unit test for a protocol with __call__.

I added some tests for callable protocols. One question that came up is whether functions should be checked against the protocol. Currently that test is failing, here is the example:

class CallableInterface(Protocol):
    def __call__(self, items: List[float]) -> List[float]: ...


class ImplementsCallableInterface1:
    def __init__(self, batch_size: int):
        self.batch_size = batch_size

    def __call__(self, items: List[float]) -> List[float]:
        return items

def implements_callable_interface2(items: List[float]) -> List[float]:
    return items

The result is:

implements_protocol(ImplementsCallableInterface1, CallableInterface) = True
implements_protocol(implements_callable_interface2, CallableInterface) = False

MiguelMonteiro avatar Oct 16 '24 11:10 MiguelMonteiro

Thanks for the feedback, sorry it's been really busy at work, I will try to get to this next week

MiguelMonteiro avatar Nov 01 '24 10:11 MiguelMonteiro

I made the necessary fixes, currently only one subtest is failing but I don't fully understand why.

@pytest.mark.parametrize(
    "expected, value",
    [
        (True, ImplementsCallableInterface1),
        (False, ImplementsCallableInterface1(1)),
        (
            False,
            implements_callable_interface2,
        ),  # TODO: switch to True once functions that implement protocol are checked correctly
        (False, NotImplementsCallableInterface1),
        (False, NotImplementsCallableInterface2),
        (False, NotImplementsCallableInterface3),
        (False, not_implements_callable_interface4),
        (False, object),
    ],
)
def test_implements_callable_protocol(expected, value):
    assert implements_protocol(value, CallableInterface) is expected

The last subtest (False, object) raises the following error instead of returning False

FAILED jsonargparse_tests/test_subclasses.py::test_implements_callable_protocol[False-object] - ValueError: Invalid or unsupported input: class=<class 'object'>, method_or_property=__call__

It does not happen in the normal (not callable) protocol tests.

MiguelMonteiro avatar Nov 01 '24 12:11 MiguelMonteiro

all should be good now

MiguelMonteiro avatar Nov 06 '24 15:11 MiguelMonteiro