See Fixed #14611 -- Added query_params to TestClient and RequestFactory. by knyghty · Pull Request #17447 · django/django · GitHub
While adding a
query_params parameter to test client and request factory methods, I hit the issue that for
HEAD requests we can already pass in a
data parameter to pass query parameters. The
data parameter is also used for form data in other request methods. Everyone (so far) agrees we shouldn’t have both for these methods.
I suggested deprecating the
data parameter for
HEAD requests. @felixxm instead prefers not adding
query_params to them and sticking with
I am going to assume that the reason for this is for backwards compatibility, but I’ll let him tell us if there are some other issues.
But the reasons I prefer the deprecation:
- All methods will have the same API, I think this is less confusing for beginners (and everyone, honestly).
- It’s also easier to use them programmatically. if you need to make lots of different types of requests with query parameters to a URL, you can iterate over the method names and call them all with the same parameters.
- I think we can be a bit more lenient on deprecating things in tests, because we shouldn’t be breaking anyone’s site with this change, only their tests.
- This kind of change could be done automatically with django-upgrade (though I’m not sure exactly how it works so do correct me if I’m wrong).
- It feels to me like the code would be easier to read and maintain.
I feel like
data are both correct names so given that
data is backwards compatible and already in a lot of folks’ muscle memory I’d probably lean that way too
I replied on the PR (…since I had it open )
tl;dr: I think having both is fine. It keeps the API consistent. It’s using both at the same time that we should guard against
I think we should warn/error if you use both params on GET/HEAD, but other than that leave it.
Let’s not remove
data. As per Aymeric’s comment on the ticket:
data argument of the test client is designed to hold simulated form data, and is automatically inserted in requests according to these rules.
Ref django-upgrade: I’d love to make it official somehow, so that folks updating would get it automatically, but until such a time we have to assume that (relatively speaking) almost nobody in the user base has heard of it or is using it.
I feel strongly about the need to support the same param name (
query_params) across all the test client HTTP methods, including
HEAD. Having worked with big and complex code bases for a long time, I can assure you there is some amount of “meta-programming” when writing tests, and this meta-programming benefits considerably from having consistency in these calls.
I’m in favor of:
- Supporting both
HEAD, with a goal of eventually deprecating
- A deprecation warning is shown when running the test suite and there are calls that use both
query_params takes precedence over
data when both are given (I think this is safe since a- developers defining
query_params are reading and using the latest code/docs, and b- the warning will be shown anyways)
@tom Thanks for raising this topic in the forum!
 Whether this is or is not a good practice and to which extent, we can discuss separately, but I have good examples where this is very handy and helps readability and maintainability of the test code.
I favour supporting both
I think a
TypeError when both are provided sounds sensible. Silent precedence would lead to tests that look like they do something with the
data argument but actually they do nothing.
I am against deprecating
data. Yes, django-upgrade could rewrite many cases, as it already does for test client headers. But as Carlton says, the use of django-upgrade is still relatively rare, and it is unofficial. (And who knows if Ruff will duplicate it at some point .) Keeping support for
data is cheap, and many tests use the positional argument format anyway, in which case the name doesn’t matter. Also, a precedent is that we didn’t deprecate support for the WSGI-style header format when adding
Re: django-upgrade and a conversation elsewhere about showcasing third party packages, I feel like it’s at least worth a mention there (when it exists) and also in any documentation (do we have much other than the release notes) around upgrading to newer versions.
Back on topic, for now I will continue with the current approach (keep both, error when using both at the same time), but it looks like there are a lot of differing opinions here, though it seems there’s a lot of support for having
query_params everywhere, and perhaps the fate of
data doesn’t need to be addressed immediately.