todofixthis / class-registry

Registry pattern for Python classes, with setuptools entry points integration!
MIT License
40 stars 8 forks source link

Type hints does not work in VSCode #53

Closed 100rab-S closed 4 days ago

100rab-S commented 1 week ago

Hi @todofixthis, I've come up with another issue πŸ˜“! In VSCode, when a class is decorated with the registry, the IDE is unable to access the type. It seems to fall back to the Any type in such cases. Please see the below example. image image

Is this an issue in VSCode or is the return type not configured well in the package? Package Version: 5.0.0 (edited)

todofixthis commented 1 week ago

Kia ora @100rab-S thanks for raising this!

Type hinting was introduced in ClassRegistry v5, so if you're using v3.0.5 you'll need to upgrade to get this functionality (note that a number of backwards-incompatible changes were introduced in v5; see Upgrading to ClassRegistry v5 for more information).

Once you've updated to ClassRegistry v5, for the type hinting to work, you can specify it via a type parameter, like this:

class Base:
    def foo(self):
        pass

# Specify the type parameter (the ``[Base]`` part) here:
dummy_registry = ClassRegistry[Base](unique=True)

@dummy_registry.register("dummy")
class Dummy(Base):
    ...

obj = dummy_registry["dummy"]
obj.f # <- Your IDE should suggest 'foo' here

This tells dummy_registry that any class registered to it derives from Base.

Let me know if you have any questions and/or if there's anything I can clarify 😺

100rab-S commented 1 week ago

@todofixthis Oh, I missed editing the above comment. I'm actually running on v5.0.0. Another question here, If I specify the base class, will the object from the registry also know about the original class and its attributes and methods? i.e.

dummy_registry = ClassRegistry[Base](unique=True)

@dummy_registry.register("dummy")
class Dummy(Base):
    def __init__(self):
          self.param1 = 'param1'

   def some_method(self):
         ...

obj = dumm_registry["dummy"]
obj.para  # <- will the IDE suggest 'param1' here?
obj.some_m  # <- will the IDE suggest 'some_method' here?
type(obj)  # <- is this 'Dummy' or 'Base'?
todofixthis commented 1 week ago

Good question. Unfortunately the IDE won't be able to infer the specific derived class. ClassRegistry is designed around the dependency injection pattern, where components expect dependencies to conform to specific interfaces, rather than specific implementations (or in the case of Python, it's usually that it derives from a specific base class, because Python doesn't really do interfaces).

tl;dr of the below: if the code relies on functionality that's specific to a derived class, then ClassRegistry probably isn't the right fit.

As an example (and one of the original use cases that motivated the creation of ClassRegistry), imagine we have a collection of data with different types – let's say fields for a contact in an address book – and we want the app to be able to render a formatted version of each piece of data.

Our data might look like this:

[
  {"type": "postal_address", "label": "home", "street_number": "123", "street_name": "Test St", /* ... */},
  {"type": "phone_number", "label": "mobile", "country_code": "64", "number": "12345678900", /* ... */ },
  // ...
]

Depending on the type of each object, the formatting logic would be different. So we implement different formatter classes that conform to an interface:

class BaseFormatter(ABC):
    def label(self, data: dict) -> str:
        return data["label"]

    def formatted_value(self, data: dict) -> str:
        raise NotImplementedError()

class PostalAddressFormatter(BaseFormatter):
    def formatted_value(self, data: dict) -> str:
        return f"{data['street_number']} {data['street_name']} ..."

class PhoneNumberFormatter(BaseFormatter):
    def formatted_value(self, data: dict) -> str:
        return f"+{data['country_code']} {data['number']}"

In order to pick the right formatter for each data type, we'll need to create a factory that instantiates the right BaseFormatter depending on the type value in the data. This is where ClassRegistry can simplify things for us:

class BaseFormatter(ABC):
    ...

registry = ClassRegistry[BaseFormatter]("data_type")

@registry.register
class PostalAddressFormatter(BaseFormatter):
    data_type = "postal_address"

@registry.register
class PhoneNumberFormatter(BaseFormatter):
    data_type = "phone_number"

Then we let the registry do the work of picking the correct formatter:

for item in data:
    # Note that in this context we don't really care which specific formatter class
    # we get; all we care about is that it derives from ``BaseFormatter``.
    formatter: BaseFormatter = class_registry[item["type"]]

    print(f"{formatter.label(item)}: {formatter.formatted_value(item)}")

But, now let's say that we wanted to add a carrier_name method to PhoneNumberFormatter:

@registry.register
class PhoneNumberFormatter(BaseFormatter):
    data_type = "phone_number"
    ...
    def carrier_name(self, data: dict) -> bool:
        ...

The challenge we have here is that carrier_name isn't part of the interface (not defined in BaseFormatter). It's also specific to working with phone numbers, so it also doesn't make sense to add it to the interface.

In this case, when we know we're working with a PhoneNumberFormatter and we want to take advantage of the phone-specific functionality, then the way to make this work without breaking IDE autocomplete, static type checking, etc. is to use a cast:

from typing import cast

for item in data:
    formatter: BaseFormatter = class_registry[item["type"]]

    if item["type"] == PhoneNumberFormatter.data_type:
        # Use ``cast()`` to tell the type checker / IDE the specific type for ``formatter``.
        cast(PhoneNumberFormatter, formatter)

        formatter.car # <- The IDE will suggest ``carrier_name``.

Note however that once we start going down the path of adding additional methods to these derived classes, we lose a lot of the value that ClassRegistry brings because we're duplicating the factory/strategy pattern within our own code (ClassRegistry picks the correct BaseFormatter class based on item["type"] but then we do the exact same item["type"] check to identify which derived class we're working with).

In this case, ClassRegistry might not be the right fit; instead we might just need the strategy pattern here:

def format_default(formatter: BaseFormatter, item: dict) -> str:
    return f"{formatter.label()}: {formatter.formatted_value(item)}"

def format_phone_number(formatter: PhoneNumberFormatter, item: dict) -> str:
    return f"{formatter.label()}: {formatter.formatted_value(item)} ({formatter.carrier_name()})"

for item in data:
    match data["type"]:
        case PhoneNumberFormatter.data_type:
            print(format_phone_number(PhoneNumberFormatter(), item))
        case PostalAddressFormatter.data_type:
            print(format_default(PostalAddressFormatter(), item))
100rab-S commented 6 days ago

Thanks @todofixthis for such an elaborate explanation. But the original question still holds. Why do we loose type information when we directly instantiate from the registry decorated class?

todofixthis commented 6 days ago

Oh. Ohhhh, I see now. Wow do I feel silly πŸ˜…

I totally misread your original message. Apologies for all the back-and-forth.

That's interesting; I'll have to look into that.

todofixthis commented 5 days ago

Right then. I've had a look at the mypy docs for specifying type information for decorator factories and the @overload decorator, and I think I've got a fix.

I'm-a go download VS Code to double-check, and if it's working I'll get a 5.0.1 release out straight away.

todofixthis commented 5 days ago

Nope, that wasn't quite it, but it's on the right track. One more change, and I think I've got it.

todofixthis commented 4 days ago

Ok, I think I've got it now #WorksOnMyMachine πŸ˜‡

Will prepare a release shortly. In the meantime you can test it out by running the following command: poetry install git+https://github.com/todofixthis/class-registry.git@develop

todofixthis commented 4 days ago

This should be resolved in v5.1.0 😺 Have a go, and let me know if it's still occurring.

100rab-S commented 3 days ago

Great πŸ₯³! It fixes the issue

image

The update also resolves another issue I was experiencing previously, which is greatly appreciated! In the previous version (v5.0.0), type hinting on the object received from the class_registry wasn't effective; it still defaulted to the Any type. Here’s an example attached below.

image

But now, this works too. That's great. Thank you πŸ™‚!

image