Code

Opened 6 years ago

Closed 6 years ago

#6165 closed (wontfix)

request.get_host() should be request.host

Reported by: eratothene Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Looking the HttpRequest class, I see all properties has plain name like: request.method, request.user, request.path. No methods that have get_smth() name, hence for holding have convention I propose to rename request.get_host() with request.host. As request.get_host() is not documented, there is no backward compatibilty problem, but the depreciated get_host() can remain for some time.

Also request.get_host() is not documented yet. I can do this as well.

Let's agree on idea and I will make patch.

Attachments (2)

patch.diff (1.3 KB) - added by eratothene 6 years ago.
Incomplete patch
patch.2.diff (862 bytes) - added by eratothene 6 years ago.
Incomplete patch

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by eratothene

Incomplete patch

Changed 6 years ago by eratothene

Incomplete patch

comment:1 Changed 6 years ago by eratothene

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I have submitted a patch, it is definitely not complete, but it shows an idea. If you like it, I will make a full version.

comment:2 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

Ticket #6119 is about documenting request.get_host()

comment:3 in reply to: ↑ description ; follow-up: Changed 6 years ago by SmileyChris

Replying to eratothene:

Looking the HttpRequest class, I see all properties has plain name like: request.method, request.user, request.path. No methods that have get_smth() name, hence for holding have convention I propose to rename request.get_host() with request.host.

I'm not sure it's worth the code churn. And we do have a get_ method already: get_full_path()

Then again, it does have a similar "feel" to request.path, so I'll leave open as a design decision. Perhaps you can bring this up in the django-dev group?

comment:4 Changed 6 years ago by eratothene

Digging futher I have found out that request.get_host() is called at least two times per each reques: in common middleware, in base handler (fix)location_header). Also number of get_host() calls grows if contrib.sites is used with RequestSite, which is also common.
As it is clear stated in the doc/request_response.txt: "HttpRequest objects. All attributes except session should be considered read-only". I propose to cache the host value as soon as it gets evaluted, as it takes quite a lot of steps to determine the host name.

comment:5 in reply to: ↑ 3 ; follow-ups: Changed 6 years ago by eratothene

Replying to SmileyChris:

Replying to eratothene:

Looking the HttpRequest class, I see all properties has plain name like: request.method, request.user, request.path. No methods that have get_smth() name, hence for holding have convention I propose to rename request.get_host() with request.host.

I'm not sure it's worth the code churn. And we do have a get_ method already: get_full_path()

Then again, it does have a similar "feel" to request.path, so I'll leave open as a design decision. Perhaps you can bring this up in the django-dev group?

I am looking at current revision of django/http/init.py is 6895, request.get_full_path() returns empty string nothing else. Sounds odd to me. Is it scheduled for removal?

comment:6 in reply to: ↑ 5 Changed 6 years ago by SmileyChris

Replying to eratothene:

I am looking at current revision of django/http/init.py is 6895, request.get_full_path() returns empty string nothing else. Sounds odd to me. Is it scheduled for removal?

Odd indeed. Go ask the group.

comment:7 in reply to: ↑ 5 Changed 6 years ago by arien

Replying to eratothene:

I am looking at current revision of django/http/init.py is 6895, request.get_full_path() returns empty string nothing else. Sounds odd to me. Is it scheduled for removal?

No, request.get_full_path() is overridden in ModPythonRequest and WSGIRequest. A comment stating that might be nice though. :-)

comment:8 Changed 6 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

I don't think this is worth changing at the moment. There is a fair bit of code that is either known to be using it or potentially using it and adding a synonym "host" property doesn't really add a lot of value. There's no real functionality improvement.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.