Opened 8 years ago

Closed 8 years ago

#3753 closed (fixed)

Allow TEMPLATE_STRING_IF_INVALID to contain a placeholder for the missing variable's name

Reported by: Matt McClanahan <cardinal@…> Owned by: adrian
Component: Template system Version: master
Severity: Keywords: Template, debug, TEMPLATE_STRING_IF_INVALID
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

It'd be handy when debugging to see the name of the undefined variable in the TEMPLATE_STRING_IF_INVALID output. To do that, the patch allows TEMPLATE_STRING_IF_INVALID to contain a single '%s' which will be replaced with the invalid variable's name.

Attachments (4)

template-invalid-varname.diff (629 bytes) - added by Matt McClanahan <cardinal@…> 8 years ago.
template-invalid-varname-doc.diff (577 bytes) - added by Matt McClanahan <cardinal@…> 8 years ago.
3753-1.diff (4.2 KB) - added by Matt McClanahan <cardinal@…> 8 years ago.
3753-2.diff (4.2 KB) - added by Matt McClanahan <cardinal@…> 8 years ago.
Rather use URL than email

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Matt McClanahan <cardinal@…>

Changed 8 years ago by Matt McClanahan <cardinal@…>

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Keywords Template debug TEMPLATE_STRING_IF_INVALID added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

Matt,

This sounds reasonable. A few changes to the existing patch(es) I'd like to see:

  1. Needs a test or two.
  2. Just check if '%s' in TEMPLATE_STRING_IF_INVALID, rather than doing the count() call. If there's more than one format string in that string, it's okay to error out, since that's a developer error that they can easily fix as part of the debugging cycle. So check if '%s' is in the string, then do the substitution if so. Python will raise an error anyway if you include more than one format marker ("not enough arguments for format string"), so you don't need to do anything extra there.
  3. You might want to consider setting a module variable to indicate if the format string is present. Settings values shouldn't ever change (it's a bug if they do), so checking it once and storing the result in a flag for subsequent times will be a performance improvement.

If you could make one combined patch for everything when you update the patch, that would be cool. Also consider including a patch for AUTHORS so that I don't have to try and spell your name correctly.

comment:3 Changed 8 years ago by Matt McClanahan <cardinal@…>

  • Needs tests unset
  • Patch needs improvement unset

As requested, tests added and suggested modifications made.

Changed 8 years ago by Matt McClanahan <cardinal@…>

comment:4 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

Changed 8 years ago by Matt McClanahan <cardinal@…>

Rather use URL than email

comment:5 Changed 8 years ago by mtredinnick

For future reference: you should not access settings.* in code that is executed at import time. Doing so means that you cannot import the module in question without haing already configured settings, which requires messy code design in the manually-configured settings case. We should allow people to follow PEP-8 formatted code practices: all imports first, then execute whataver is required (settings configuration, if needed).

I've altered the patch slightly so that settings are only accessed when required (inside Template methods).

comment:6 Changed 8 years ago by mtredinnick

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

(In [5167]) Fixed #3753 -- Allow optional display of invalid variable name in
TEMPLATE_STRING_IF_INVALID. Thanks, Matt McClanahan.

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