Description

There are references to methods/functions of both names get_query_set and get_queryset which I think means all the same in all cases, should this not have one and the same name? I would suggest conforming to get_query_set.

Firstly, get_queryset() is consistent with PEP8, so that *should* be the preferred name.

Secondly, if we changed to using get_query_set() in Class-based views, we would have a situation where we have self.queryset, but self.get_query_set(). Inconsistency is a bad thing, but internal inconsistency is worse.

So, it's an annoying inconsistency, but it's one we'll live with until 2.0.

All tests pass, albeit with a lot of expected deprecation warnings. The issue is this doesn't provide an upgrade path for people who override these methods in class inheritance, their methods will be silently ignored.

I'm not sure how to proceed at this point, should we close this ticket or go forward and document the change as backward incompatible?

Unless I'm missing the point, I think this decorator suffers from the same issue as mine: since all calls to get_query_set in Django core are changed to get_queryset, the user defined get_query_set will never be called.

Also I went the method decorator route because there are a few more queryset getter inconsistencies such as BaseModelAdmin.queryset, get_prefetch_query_set, etc.

Should we aim at removing the last few occurrences of query_set for the sake of consistency? There are only a few variables and methods left after this patch, mostly private APIs.

Also BaseModelAdmin.queryset is what brought me to this ticket: I wasted a bunch of time and resorted to some crazy hacks to access the admin queryset because I failed to notice this method since I was looking for a get_queryset. All similar methods in ModelAdmin have a get_ prefix.

For completeness I'll also mention ListFilter.queryset but I believe this one is acceptable since it is consistent with the surrounding methods.

I think this patch is a good opportunity to solve the rest of the queryset accessor inconsistencies.

according to very quick testing this doesn't work similarly in master and after patch.

It is likely possible to make mixins work with some mro introspection. I don't think the direct assignment case needs to be supported, it is likely extremely rare.

Apart of the above cases I can't think of any case that will not work - as long as each class in the inheritance chain is subclass of Manager they will get both the get_queryset and get_query_set methods, and things will work. But, Python allows for some crazy constructs so I am not surprised if other problematic constructs are found.

I am not sure if this kind of renaming is the best idea in the world. But, I don't care enough to actually put a -0 here (let alone -1). I guess we could do this renaming, and if we want to do other similar ones, then lets first wait for 1.6 and see how this one actually worked out. Of course, if something similar has been done already and it worked well, then put me into +0 camp.

Added a new patch that doesn't solve the mixin issue described by @akaariai but provide a generic of dealing with method rename deprecations.

It should be easier now to use it for the get_prefetch_query_set -> get_prefetch_queryset and queryset -> get_queryset migration if it should ever happen.

Will wait for another core developer to chime in about whether or not it's worth the trouble to go forward with this.

Personally I think the change is worth it for the sake on consistency. Pushing the first rename (get_query_set -> get_queryset) early into the 1.6 life cycle should give us a clear view over the viability of this kind of refactoring.

@loic84, I won't have time to work on this in next few weeks. Feel free to tackle the __mro__ issues :)

@charettes, I somehow failed to notice your patch with the factory and I came up with something slightly different but in the same spirit. It shouldn't be too hard to replace it with the factory if you like it better just let me know.

The metaclass works a bit differently though; no exception is raised when both the old and the new methods are present, since it actually rely on the existence of both.

The logic goes as follows:

Define the new method if missing and complain about it.

Define the old method if missing to assure backwards compatibility.

Complain whenever an old method is called.

The full test suite runs without warning (with -Wall). I also updated every reference to the deprecated methods in the docs except in the releases subfolder.

I'm testing one of our apps with 1.6 beta 1, and went ahead and changed queryset to get_queryset in our model admins and custom list filters. The former works fine, however, it turns out that list filters is still using the old queryset instead of get_queryset as I expected. Shouldn't this cleanup patch change that as well?

@loic84, @charettes: Yes, I'm referring to ListFilter.queryset. I now see that you discussed that method and kept it unchanged for internal consistency. I am however in disagreement with that choice, as people will be thinking "how do I get the queryset? I'm guessing get_queryset as that's whats used everywhere else", not "I'm guessing queryset, as that matches the other methods on list filters"... IMHO, of course, but I was bitten by this exact thing implementing my own custom list filter.

@asteinlein I disagree with your people will be thinking assertion. I think most of Django users will be referring to ​the documentation when subclassing SimpleListFilter since it's a newly introduced feature.

However I agree that the whole ListFilter method names (missing get_) might be confusing for people used to override BaseModelAdmin methods thus I suggest you open new ticket suggesting to prefix ListFilter methods. It should be easy to write a patch using the RenameMethodsBase and we should be more confident shipping another method rename once people got their hands on Django 1.6 and played with the get_query_set one.