Drop @register_assistant decorator

[pamella]

Issues

Resolves #97

Description

Drop @register_assistant decorator

Screencast.from.19-06-2024.11.18.39.webm

Screencast.from.19-06-2024.11.16.55.webm

Discussion notes

Note

Hey there, I opened this draft to get your thoughts on the new approach.

[Click to see] Resolved discussion notes

1. We could create a get_assistant_cls at django_ai_assistant/helpers/assistants.py, but we would need to use an alias when importing it in django_ai_assistant/helpers/use_cases.py or renaming the existing get_assistant_cls

   def get_assistant_cls(assistant_id: str) -> dict[str, type[AIAssistant]]:
       return AIAssistant._get_assistant_cls_registry()[assistant_id]

We opted for the alias.

2. Without the decorator, the test_list_assistants_with_results is currently failing because it returns all AIAssistant classes created within tests/. Could it be an issue?

   

We used __init_subclass__ in AIAssistant, and @pytest.fixture(scope="module", autouse=True) in the test files.

[pamella]

For this usage, we could have a get_assistant_cls helper but there are a few side effects (please see the PR description for more details).

[fjsj]

Overall LGTM.

We could create a get_assistant_cls at django_ai_assistant/helpers/assistants.py, but we would need to use an alias when importing it in django_ai_assistant/helpers/use_cases.py or renaming the existing get_assistant_cls

Alias is fine on the use_cases import.

Without the decorator, the test_list_assistants_with_results is currently failing because it returns all AIAssistant classes created within tests/. Could it be an issue?

That's bad because it makes unit test harder... is the problem because the __subclasses__ call isn't updated? Perhaps if we use __init_subclass__ we can solve that? See: https://stackoverflow.com/a/53101259

[pamella]

Alias is fine on the use_cases import.

All right, I'll go for it.

That's bad because it makes unit test harder... is the problem because the __subclasses__ call isn't updated? Perhaps if we use __init_subclass__ we can solve that? See: https://stackoverflow.com/a/53101259

I can refactor the code to use __init_subclass__, but it doesn't solve the unit testing problem. I'm checking alternatives.

[pamella]

I can refactor the code to use __init_subclass__, but it doesn't solve the unit testing problem. I'm checking alternatives.

I added the __init_subclass__ to the AIAssistant, and @pytest.fixture(scope="module", autouse=True) in the test files.
https://docs.pytest.org/en/7.1.x/how-to/fixtures.html?highlight=scope#fixture-scopes

[fjsj]

Can we remove the re.match from the init?

[pamella]

I added the verification in the __init_subclass__ method because it occurs at the time the subclass is created, not when an instance is initialized (__init__). This way, inconsistencies in the id could be detected earlier, preventing the creation of invalid subclasses.

[fjsj]

yes, that's good, but can we remove the one from __init__?

[pamella]

Yes, will do. I will move all checks from __init__ to __init_subclass__

[fjsj]

Since we're adding classmethods to AIAssistant, maybe just make these classmethods too? This will simplify the API.

[pamella]

Sure, I actually prefer this way too

[pamella]

Done (commit b04abb4)

[fjsj]

Overall LGTM, but please check comments.
Also, please update docs.

[amandasavluchinske]

Nice! Much cleaner :)