Opened 7 years ago

Closed 7 years ago

#6578 closed (invalid)

Strange code introduced in changeset 7089

Reported by: Grzegorz Lukasik <hauserx@…> Owned by: nobody
Component: Template system Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

http://code.djangoproject.com/changeset/7089

 	293	            try: 
 	294	                if nodelist.contains_nontext: 
 	295	                    raise AttributeError 
 	296	            except AttributeError: 
 	297	                raise TemplateSyntaxError("%r must be the first tag in the template." % node) 

raising exception and then catching it immediately doesn't look right.

Attachments (1)

template_simplify.patch (887 bytes) - added by Grzegorz Lukasik <hauserx@…> 7 years ago.

Download all attachments as: .zip

Change History (3)

Changed 7 years ago by Grzegorz Lukasik <hauserx@…>

comment:1 Changed 7 years ago by Grzegorz Lukasik <hauserx@…>

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by mtredinnick

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

The current code is correct. We need to catch the attribute error because nodelist might not have an attribute called contains_nontext (read the commit message for why that's the case). Therefore, to avoid code duplication, since we want the same error handling, we just make sure we leap into the exception catcher. This is all in the error path, so it's not a performance penalty that's paid by correct code. It's also fairly idiomatic Python: if a block of code has an exception handler and you need the same error handling, jump into the exception handling portion by raising the right exception. Used throughout the standard library in quite a few places.

So, thanks for the review, but it's correct.

Note: See TracTickets for help on using tickets.
Back to Top