netzkolchose / django-computedfields

Provides autogenerated autoupdated database fields for model methods.
MIT License
94 stars 14 forks source link

Alternative / more readable syntax #135

Closed olivierdalang closed 1 year ago

olivierdalang commented 1 year ago

Hey ! Still heavily using this great package in many projects ! Thank you very much for that !!

My only minor regret when using it is how it makes my model declarations less readable, because the decorator syntax forces to mix fields with logic. It's particularly true with more complex models that have long computation logic and/or many computed fields. Having easy to read models is definitely a big plus in Django projects.

Wouldn't it be possible to provide an alternative way to declare computed fields that has less impact on readability ?

I didn't really thought this through in terms of implementation, but below are a few ideas about how it could look like. IMO they all better convey what exactly the library does (e.g. automatically populate computed values, with no impact on the data model itself).

Current

class MyModel(ComputedFieldsModel):
    firstname = models.CharField(max_length=32)
    lastname = models.CharField(max_length=32)
    @computed(
        models.CharField(max_length=32), depends=[("self", ["firstname", "lastname"])]
    )
    def fullname(self):
        # ...
        return result
    birthday = models.DateField()

Alternative A: a new computes decorator

class MyModel(ComputedFieldsModel):
    firstname = models.CharField(max_length=32)
    lastname = models.CharField(max_length=32)
    fullname = models.CharField(max_length=32)
    birthday = models.DateField()

    @computes("fullname", depends=[("self", ["firstname", "lastname"])])
    def make_fullname(self):
        # ...
        return result

Alternative B: wrapping the field definition in a function

class MyModel(ComputedFieldsModel):
    firstname = models.CharField(max_length=32)
    lastname = models.CharField(max_length=32)
    fullname = ComputedField(
        models.CharField(max_length=32),
        func="_fullname",
        depends=[("self", ["firstname", "lastname"])],
    )
    birthday = models.DateField()

    def _fullname(self):
        # ...
        return result

Alternative C: definition computed fields in Meta

class MyModel(ComputedFieldsModel):
    class Meta:
        computed_fields = {
            "fullname": ("_fullname", [("self", ["firstname", "lastname"])])
        }

    firstname = models.CharField(max_length=32)
    lastname = models.CharField(max_length=32)
    fullname = models.CharField(max_length=32)
    birthday = models.DateField()

    def _fullname(self):
        # ...
        return result

Maybe there would even be a way for the function to be just any callable and not necessarily a method on the Model itself. Don't we know know the arguments to the callable thanks to the "depends" configuration ? This way we could even use existing callables (such as built-in templatetags like slugify or striptags). It would look like this (would work well for alternative B as well):

def make_full_name(firstname, lastname):
    # ...
    return result

class MyModel(ComputedFieldsModel):
    class Meta:
        computed_fields = {
            "fullname": (make_full_name, [("self", ["firstname", "lastname"])])
        }

    firstname = models.CharField(max_length=32)
    lastname = models.CharField(max_length=32)
    fullname = models.CharField(max_length=32)
    birthday = models.DateField()

Another argument for this is that it would likely play better with type hints (not tested though).

Thanks again and feel free to close the issue if you don't like the idea, as it's not critical.

jerch commented 1 year ago

Hmm, at a first glance all should be possible - more or less easy (some impl details might be harder).

If I got your intention right - you want a better separation of field declarations vs. the functions itself. Well, when I shaped the decorator as it is now, I came from the opposite direction - from property functions. So it was kinda obvious to stick close to the property declaration scheme, and just enhance it with fields (and from there the whole resolver madness took off).

If its just for getting the fields closer to your block of field declarations, maybe calling the decorator explicitly with a function reference already helps? Something like this (untested):

def some_func(inst):
    ...
    return 'some_string_stuff'

class MyModel(...):
    fieldXY = computed(models.CharField(...), depends=[...])(some_func)

(Not quite sure if this works out of the box, might need to be a class method...)

Putting more weight on that "separation of concern" idea, I'd prolly shape things more like your Alternative B, maybe even with args/kwargs expansion, so a pure "outer" function could be used. Such an interface would be more like that of signal handlers. Not sure though about performance implications. About your other Meta-based ideas - it occured to me once or twice, that custom metas would help at other ends, but so far I refrained from entering that next level of configuration complexity, mainly for that complexity reason.

So sure I am open to these ideas, as long as they dont change resolver internals.

Edit: I think it should be possible to use the wrap function here https://github.com/netzkolchose/django-computedfields/blob/e9985a6d0458753c0ac9aa1a90d97b8234a5e7da/computedfields/resolver.py#L783-L795 as a ComputedField factory function to form this declaration style:


class MyModel(...):
    fieldXY = ComputedField(some_func, models.FieldXY(...), depends=[...])
jerch commented 1 year ago

@olivierdalang I've thought abit about your ideas here. Prolly gonna add the factory method ComputedField as described in my last post. Thats very easy and straight forward to add.

Don't we know know the arguments to the callable thanks to the "depends" configuration ? This way we could even use existing callables (such as built-in templatetags like slugify or striptags). It would look like this (would work well for alternative B as well):

Yes, the argument splitting would also help with another issue I currently have with computed fields - currently there is no way to tell, if the depends rules contains nonsense (beside basic syntax and semantics checks against existing paths and fields), which will lead to faulty update behavior, if the function relies on different dependencies than actually annotated in depends. So splitting the arguments and pre-pulling their values might be the better interface in the long run, but I still hesitate to implement that, as its rather involved to get done right. Also such a rigid interface would be very opinionated on how to access values, which might restrict more complicated model setups too much (ppl cannot use self-declared fancy Prefetch shortcuts anymore etc). Edit: There might be a way to circumvent some restrictions here by templating the value accessors into depends directly - but no clue yet, how to shape that in a readable manner without creating a template language on top of the ORM.