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

Enhance DX & Type Safety with Typed Loading Methods #109

Open
ramppdev opened this issue Jun 20, 2024 · 4 comments
Open

Enhance DX & Type Safety with Typed Loading Methods #109

ramppdev opened this issue Jun 20, 2024 · 4 comments

Comments

@ramppdev
Copy link

Currently the loading methods (e.g. audobject.from_yaml_s) return an instance of audobject.Object, causing the loss of specific type information for LSPs and tools like mypy.

It might be a good idea to add an optional type parameter (e.g. instance_of) to improve static analysis and type verification.
In addition, this could ensure that the loaded object is actually an instance of the expected type, providing an additional layer of safety and reducing potential runtime errors.

This could default to instance_of=audobject.Object such that full backwards compatibility should be provided (assuming no parameter with the same name was passed).

Example with the loss of typing information:

import audobject


__version__ = "1.0.0"  # pretend we have a package version


class TestObject(audobject.Object):
    def some_method(self, some_arg: str) -> None:
        print(some_arg)


test_object = TestObject()  # test_object is an instance of TestObject
test_object.some_method("hello")  # LSP / mypy information avaliable here

test_yaml = test_object.to_yaml_s()

test_object2 = audobject.from_yaml_s(
    test_yaml
)  # test_object2 is an instance of TestObject but only known as audobject.Object

test_object2.some_method(123)  # no LSP / mypy information available here

Example with added typing information:

from typing import Type, TypeVar


T = TypeVar("T", bound=audobject.Object)


def from_yaml_s_typed(
    yaml_s: str,
    instance_of: Type[T] = audobject.Object,
    **kwargs,
) -> T:
    instance = audobject.from_yaml_s(yaml_s, **kwargs)
    if not isinstance(instance, instance_of):
        raise ValueError(...)
    return instance


test_object3 = from_yaml_s_typed(
    test_yaml,
    instance_of=TestObject,
)  # test_object3 is an instance of TestObject and known as such

test_object3.some_method(123)  # LSP / mypy information available here
# mypy error: Argument 1 to "some_method" of "TestObject" has incompatible type "int"; expected "str"  [arg-type]
@ramppdev ramppdev closed this as completed Oct 5, 2024
@hagenw
Copy link
Member

hagenw commented Oct 10, 2024

As you closed it, did you found another solution?

@ramppdev
Copy link
Author

As you closed it, did you found another solution?

Oh i think i closed it by accident, but no, at the moment we just assume that the user is not tampering with their serialized YAML files.
At the same time we could always add such a check ourselves.
Also on second thought it is questionable if the library itself should validate the expected class, or if this should be done by the user.

@hagenw
Copy link
Member

hagenw commented Oct 10, 2024

Yes, I think it would be very nice to have correct typing, but it is not a nice solution if the user has to provide the expected class as argument.

@ramppdev
Copy link
Author

it is not a nice solution if the user has to provide the expected class as argument.

I agree, maybe there is a better solution. I will reopen the issue.

@ramppdev ramppdev reopened this Oct 10, 2024
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

No branches or pull requests

2 participants