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

Adding GeneratedField to supported types for RangeNumericFilter #824

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yarimiz
Copy link

@yarimiz yarimiz commented Oct 23, 2024

I have a use case where I wish to have a RangeNumericFilter for a GeneratedField.
This was not allowed until now. This PR is to allow this configuration.

I had not added any extra validations to check whether the actual value of the GeneratedField is numeric, as I don't think this should be done by unfold. Let me know if you think otherwise.

@lukasvinclav
Copy link
Contributor

What type is GeneratedField in database?

@Zerotask
Copy link

What type is GeneratedField in database?

depends on what you pass for output_field. For example output_field=models.CharField(max_length=16) would be a VARCHAR(16)

@lukasvinclav
Copy link
Contributor

if the column type is variable, how we are checking if this field can be used for numeric filtering?

@yarimiz
Copy link
Author

yarimiz commented Oct 25, 2024

One way we can resolve this is to make sure the output_field of the GeneratedField is one of the already supported field types (DecimalField, IntegerField, FloatField, AutoField)

@lukasvinclav
Copy link
Contributor

exactly, if you can, please update the PR

@yarimiz
Copy link
Author

yarimiz commented Oct 25, 2024

Sure, working on that now

@lukasvinclav
Copy link
Contributor

please ensure that your change is going to be backward compatible as well. I guess on django pre 5.x import fill fail.

@yarimiz
Copy link
Author

yarimiz commented Oct 25, 2024

what is the best appraoch you suggest for safe importing? I thought either importing only if django is >=5.0 at the top level, or do the whole importing the GeneratedField.output_field checks inside a try-except and handling the ImportError if raised.

@lukasvinclav
Copy link
Contributor

In the past I used the approach below but if you are aware of something more elegant, I'm open to incorporate it everywhere:

https://github.com/unfoldadmin/django-unfold/blob/main/src/unfold/admin.py#L59-L65

@yarimiz
Copy link
Author

yarimiz commented Oct 25, 2024

I don't think my appraoch is more elegant, but just might be more "local"

try:
    from django.db.models import GeneratedField
     if isinstance(field, GeneratedField) and isinstance(
        field.output_field, (DecimalField, IntegerField, FloatField, AutoField)
    ):
        raise TypeError(
            f"Class {type(field.output_field)} of the output_field is not supported for {self.__class__.__name__}."
        )
except ImportError:
    pass

Tell me which you prefer and I'll implement it

@lukasvinclav
Copy link
Contributor

Your snippet looks good as well.

@yarimiz
Copy link
Author

yarimiz commented Oct 25, 2024

Please take a look at the final code when you have the time :)

@lukasvinclav
Copy link
Contributor

I played with simpler check for backward compatibility. What do you think about the solution below. Would you mind to test it?

if not isinstance(
    field.output_field if hasattr(field, "output_field") else field,
    (DecimalField, IntegerField, FloatField, AutoField),
):
    raise TypeError(
        f"Class {type(self.field)} is not supported for {self.__class__.__name__}."
    )

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.

3 participants