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

Extend the forward reference handling to more complex boundary cases #132

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

daveraja
Copy link
Collaborator

@daveraja daveraja commented Feb 14, 2024

I've extended the postponed type annotations for some more complicated (boundary) cases. For example, when the Predicate definition with type annotation is defined in a non-global scope (eg. within a function) and is referring to another predicate definition of a different scope. While some of these cases are probably not something anyone would (or should) do, it is good to have it working in case.

@florianfischer91 let me know if you have any thoughts. I find this postponed annotations and the from __future__ import annotations stuff to be quite a mess. It seems that there is no clear way to deal with it that doesn't involve some sort of hack; such as accessing undocumented APIs (eg. typing._eval_type()) or complicated workarounds for specific versions of Python.

Should now work when the Predicate definition with type annotation is defined in a non-global
scope (eg. within a function) and is referring to another predicate definition of a different
scope.
For these Python versions the _eval_type() function fails on ForwardRefs that are quoted
strings. So first strip any quotes from type annotation strings.
@daveraja
Copy link
Collaborator Author

Note, I've modified slightly the logic of mixing type annotations with explicitly defined fields. If a type annotation is used for one field then it must be used for all. But you can use a type annotation and still override the behaviour with an explicit field() declaration. This allows the user to write their own BaseField class to provide some specific Python to Clingo translation, but still maintain the type checking features for linting, etc.

@daveraja daveraja merged commit 24d053f into master Feb 14, 2024
34 of 36 checks passed
@florianfischer91
Copy link
Collaborator

Sry for the late response.
Yeah dealing with types in python is always a bit ugly and even more if you have to support multiple python versions.
Supporting non-global scope is a good idea. I can remember that there was something like this in Clingraph (don't know if this still the case) when you dynamically want to define classes. I have also tried this and it works 👍
I also like the change that a user either has to use annotations for all fields or not use annotations at all.

@daveraja
Copy link
Collaborator Author

Sry for the late response. Yeah dealing with types in python is always a bit ugly and even more if you have to support multiple python versions. Supporting non-global scope is a good idea. I can remember that there was something like this in Clingraph (don't know if this still the case) when you dynamically want to define classes. I have also tried this and it works 👍 I also like the change that a user either has to use annotations for all fields or not use annotations at all.

Cool. Thanks for responding and testing it out.

@daveraja daveraja deleted the postponed_annotations branch February 19, 2024 10:55
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

Successfully merging this pull request may close these issues.

2 participants