Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #30310 -- Added support for looking up HttpHeaders.headers using underscores. #11165

Merged
merged 1 commit into from May 9, 2019

Conversation

Troon
Copy link
Contributor

@Troon Troon commented Apr 3, 2019

Added getitem() to HttpHeaders.headers to include underscore-to-hyphen lookup for use in templates.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Troon Thanks for this patch 👍 Tests are required.

django/http/request.py Outdated Show resolved Hide resolved
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this not just be a one-liner:

def __getitem__(self, key):
    return super().__getitem__(key.replace('_', '-'))

?

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. It is adding something to django.http.request that is a workaround for something you are trying to do in a template.

You can solve this in your project by adding a template filter for dictionary lookups: {{ headers|lookup:'foo-bar' }}. There are plenty of examples of how to implement such a filter that you can search for.

(In addition to tests this would also need a Trac ticket.)

django/http/request.py Outdated Show resolved Hide resolved
@Troon
Copy link
Contributor Author

Troon commented Apr 3, 2019

Could this not just be a one-liner:

def __getitem__(self, key):
    return super().__getitem__(key.replace('_', '-'))

?

No: it needs to work for published new behaviour ("user-agent") and in templates ("user_agent").

I'm trying to figure out how to update the branch, as I now have this:

    def __getitem__(self, key):                                                 
        """                                                                     
        Django template variables cannot contain hyphens, so to allow use       
        of the headers object in templates, we also try to look up the          
        variable with underscores. For example, use headers['foo-bar']          
        in views, and {{ headers.foo_bar }} in templates.                       
        """                                                                     
        try:                                                                    
            return super().__getitem__(key)                                     
        except KeyError:                                                        
            return super().__getitem__(key.replace('_', '-'))                   

@carltongibson
Copy link
Member

>>> "user-agent".replace('_', '-')       
'user-agent'

@ngnpope
Copy link
Member

ngnpope commented Apr 3, 2019

No: it needs to work for published new behaviour ("user-agent") and in templates ("user_agent").

Actually @carltongibson is correct. The header values are always hyphenated, so always converting underscores to hyphens will allow both strings to work.

@Troon
Copy link
Contributor Author

Troon commented Apr 3, 2019

>>> "user-agent".replace('_', '-')       
'user-agent'

Heh, of course. Sorry.

@carltongibson
Copy link
Member

carltongibson commented Apr 3, 2019

So Nick's point is the one I raised on the ticket: the standard approach here is to implement just such a filter. (Or to do it in get_context_data() in the view...)

So here's the question for reviewers: as an affordance here, is this an addition we'd like to add? ("No, wontfix", is an allowed answer there.)

of the headers object in templates, we also try to look up the
variable with underscores. For example, use headers['foo-bar']
Django template variable lookup precludes hyphens. By converting
underscores to hyphens here, we allow the use of headers['foo-bar']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid use of "we". (See Coding Style guidelines 👍)

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @carltongibson and @felixxm on this. It is a trivial two line addition.

django/http/request.py Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Added __getitem__() to HttpHeaders.headers Fixed #30310 -- Added support for looking up HttpHeaders.headers using underscores. Apr 8, 2019
@rixx
Copy link
Contributor

rixx commented Apr 14, 2019

I'd like to see this change in Django (once tests have been provided, of course). Saying "here is a new easy way to interface with headers!" and then having it be hard to use in templates defeats the purpose for at least a large subset of Django users, and this change alleviates the problem at basically no cost.

@carltongibson
Copy link
Member

Hi @pope1ni, @felixxm. I'm inclined to accept this, make a the couple of cosmetic changes and pull it in. (I've ummed and arghed about but in the end think pragmatism beats purity here.) If you don't object I'll proceed, otherwise we can widen the discussion to the list and see what people think? (I'm very happy with either, so if you do hate it shout now. 🙂)

@ngnpope
Copy link
Member

ngnpope commented Apr 24, 2019

Hi @carltongibson. Fine by me - thinking on this some more, it feels generally useful. There's still an outstanding comment regarding simplifying the docstring and a missing release note.

As this is a straightforward two line addition I wouldn't be adverse to it being backed to 2.2 where request.headers was introduced, if only to prevent reports of this issue recurring in the months ahead until 3.0 arrives. But I guess that's up to you and @felixxm to decide!

@carltongibson
Copy link
Member

Hey Nick. Thanks for the quick reply.

I'm not sure about back porting... In the strictest of worlds it doesn't really qualify. (It's a new feature on a new feature, rather than a bug on a new feature...)

@ngnpope
Copy link
Member

ngnpope commented Apr 24, 2019

In the strictest of worlds it doesn't really qualify.

Agreed. Was just putting it out there that this one might come back to be annoying...

@carltongibson carltongibson merged commit a3a4f5c into django:master May 9, 2019
@carltongibson
Copy link
Member

Hey @Troon. Thanks for the contribution! Good job! Welcome aboard! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants