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

On Tuesday, 20 October 2015 16:27:10 UTC+11, David Filipovic wrote:
>
> 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.
>
>
> On Sunday, October 18, 2015 at 9:56:51 PM UTC-4, Josh Smeaton wrote:
>>
>> > 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
>>
>> On Saturday, 17 October 2015 14:08:49 UTC+11, David Filipovic wrote:
>>>
>>> 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
>>>
>>> On Thursday, October 15, 2015 at 11:21:40 PM UTC-4, Josh Smeaton wrote:
>>>>
>>>> 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,
>>>>
>>>> On Friday, 16 October 2015 05:43:48 UTC+11, David Filipovic wrote:
>>>>>
>>>>> 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
>>>>>
>>>>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/7fc0508a-0220-478e-b4ba-b525f5c0a518%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to