Contributing to a module that does not seem to be covered by tests extensively (djanog.db.models.expressions)

[David Filipovic]

Hi all,

I have recently stumbled upon a very useful feature missing from `django.db.models.expressions`.

I came up with a patch and followed the guidelines to create my ticket: https://code.djangoproject.com/ticket/25556
and pull request: https://github.com/django/django/pull/5431

One thing I'm confused about is the fact that I would be adding new classes/functionality to the system (so I would really want to write tests for it), however similar classes in the same module seem not to be covered by tests at all. For instance, the only test for `django.db.models.expressions.Date` I could find was the one testing its `__repr__` method, which doesn't seem all that useful.

One thing that's noticeable is that a lot of the more complex query expressions present there (like Date and DateTime) are not really covered by the documentation either (even though Date is used in `QuerySet.dates()` which is well documented). Are these features considered undocumented, and is there a plan to make them officially supported (because they're really useful)?


One other thing I wanted to inquire about is what is the best naming practice for django when adding new classes/methods/functions? I couldn't find anything on this.

Thank you so much. I hope I can contribute and help make django better!

Best regards,
     David Filipovic

[Josh Smeaton]

Hi David,

There are a few undocumented expressions in the expressions module because they aren't intended to be used by the public. The Date and DateTime expressions you reference were actually moved from elsewhere (if memory serves) during the expressions refactor. They were previously very thin data wrappers used to represent dates in columns. Some of the functionality was pulled out of query.py and moved to inside the expression. They're the result of a refactor, which is why they weren't given their own tests I assume. However, they should probably have been tested before.

If you can make a case for making these types public then please do so. By making them public we'd document them and write some tests to ensure they do what they say they do. From a cursory inspection these two particular types look like helper classes for F() expressions, so I'm not exactly sure of their public utility. So if you'd like to see them made public (or partially public - as building blocks for other expressions), then please show how you'd use them and we can consider doing so. This goes for any other type in the expressions/lookups namespaces by the way. Unless utility is immediately apparent we'd default to private, but always open to making something public.

Regarding coding style, there's https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/. More generally there's https://docs.djangoproject.com/en/dev/internals/contributing/ and https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/. Other than that, bikeshedding goes a long way towards naming (code review focusing on trivialities).

It'd be really great to have some more contributors helping with expressions and the ORM in general so, welcome! The mailing list is good for getting some long form answers, but feel free to join us on IRC at #django-dev to bounce some ideas off or get some more clarification.

Regards,

[David Filipovic]

Hi Josh,

Thanks so much for your input, both here and on the ticket I referenced.

You were correct, MonthTransform did work. Essentially, this piece of code worked in django 1.9:

    MyModel.objects.filter(date1__month=MonthTransform('date2'))

However, the way I see it here, the right hand side lookup interface is all broken up and scattered across multiple modules and classes, many of which are undocumented.

I am of the opinion that there should be a unified right hand side lookup interface, whether it resided in lookups or expressions, but it should be possible to just use one class for all types of right hand side expressions/lookups.

The left hand side lookups work just fine and are able to figure out everything about the type of field and the type of lookup and translate that into sql, so why shouldn't the right hand side ones be able to do the same? 

The documentation does say that the F() object represents the value of a model field, but is there a reason why it couldn't be the universal right hand side lookup operator, inclusive of 'subfields' (in a manner of speaking) such as year, month and day on DateFields (essentially, a subset of lookups allowable on the left hand side, such that gt would not be allowed, for instance), so that the following would work:

    MyModel.objects.filter(date1__month=F('date2__month'))

Additionally, is there a reason why Combinable/F couldn't figure out its own output_field type (at least in some cases? e.g (DateField - DateField) --> DurationField) , as opposed to having to use an ExpressionWrapper, which makes the interface slightly more complicated?

What do you think, Josh?

Best regards,

     David Filipovic

[charettes]

Hi David!

As Josh said it's great to see another contributor interested in the ORM.

There was a recent discussion on this mailing list about an ORM query syntax
enhancement[1] that you might find interesting. From what I remember there
was a proposal similar to yours.

Simon

[1] https://groups.google.com/forum/#!topic/django-developers/W0OYXhavY68

[Josh Smeaton]

> However, the way I see it here, the right hand side lookup interface is all broken up and scattered across multiple modules and classes, many of which are undocumented.

Yes, that's what we should try to fix in 1.9 before it reaches Beta. I'm mostly interested in the DateTransform classes for the moment, since they were only recently added and we have an opportunity to improve them.

Firstly, I think they should be renamed. I'm not sure if we should go for Month() or ExtractMonth() or something else entirely. We don't have the opportunity to do an Extract(lookup='month') type of API just yet which may be a better overall API. Does anyone have thoughts on naming?

Once they've been named, we should consider where the best place for them to reside should be. I'm guessing db/functions.py with the rest of the dbfunctions, but they could easily go into a submodule type arrangement, either lookups/datetime.py or functions/datetime.py.

Then the ones we're making public need to be documented. This should be fairly straightforward once we've achieved the above.

We need to get this done quickly in order to make 1.9. The beta will be released next week, so the timeline is the next few days. Opinions definitely welcome.

> MyModel.objects.filter(date1__month=F('date2__month'))

This is an option in the future. Charettes has linked to a discussion on alternative syntax. So, for the moment, let's just get the object based right hand side documented, and we can work on alternatives later.

> Additionally, is there a reason why Combinable/F couldn't figure out its own output_field type (at least in some cases? e.g (DateField - DateField) --> DurationField)

If you can find an appropriate place for this logic to live then I'm all for it. ExpressionWrapper is cumbersome, but it's also necessary for user based combinations where the base F/Combinable/Expression has no knowledge of output_type combinations.

Cheers

[David Filipovic]

Hey, Josh,

The 1st Beta was released today. Should we still try to make it into 1.9, or is it too late? 

I can get started on the move tomorrow (Tuesday) night. I suggest:

1. moving all date/time related transforms into lookups/datetime.py, and leaving all the other ones in lookups/__init__.py.

(this is important because these transforms still have to be registered as lhs lookups in __init__.py, in order not to break any existing functionality)

2. Renaming DateTransform to DateTimeExtract (to match lower-level API) [possibly split into DateExtract and TimeExtract] and make everything work through that interface. Regarding this, you mentioned we don't have an opportunity to do Extract(lookup='month') type API. Is there a reason for this? It could be easily done with the __new__ magic method and if we need all the other transform classes available, for some reason, we could just initialize them with a DateTimeExtractFactory, for instance. So something like: MonthTransform = DateTimeExtractFactory('month').

That way we can just make the DateTimeTransform public, it would be easy to document and testing would be a breeze. What do you think?

I'll create a new ticket and get started on this tomorrow after work. I expect to have a PR compiled by Wednesday night, but take a little longer.

[Josh Smeaton]

Hi David,

Unfortunately we've missed the cutoff for 1.9, this will have to wait to be released with 1.10. I didn't want to delay the beta date with what could potentially be a drawn out patch (validating names, locations of classes, accuracy of docs). Further, we probably shouldn't have added an argument to DateTimeExtract (lookup_name?) during an alpha.

1. I agree, they are probably the correct places to put those functions/classes. We can then import lookups.datetime.* into lookups so that they are correctly registered.

2. We can definitely create an Extract(field, lookup='month') type for 1.10. My objection was changing the signature of that class during an alpha. Now that we're targeting 1.10, we can go ahead with this.

I don't think we need to create a class factory though, since we're only going to be using this type in a small finite set of classes.

class Month(Extract):

    lookup = 'month'

Month(field)

and 

Extract(field, lookup='month')

Should be equivalent and both be options.

Cheers