Opened 10 years ago

Closed 9 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 Holovaty
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@…> 10 years ago.
template-invalid-varname-doc.diff (577 bytes) - added by Matt McClanahan <cardinal@…> 10 years ago.
3753-1.diff (4.2 KB) - added by Matt McClanahan <cardinal@…> 9 years ago.
3753-2.diff (4.2 KB) - added by Matt McClanahan <cardinal@…> 9 years ago.
Rather use URL than email

Download all attachments as: .zip

Change History (10)

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

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

comment:1 Changed 10 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: UnreviewedDesign decision needed

comment:2 Changed 9 years ago by Malcolm Tredinnick

Needs tests: set
Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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 9 years ago by Matt McClanahan <cardinal@…>

Needs tests: unset
Patch needs improvement: unset

As requested, tests added and suggested modifications made.

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

Attachment: 3753-1.diff added

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

Triage Stage: AcceptedReady for checkin

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

Attachment: 3753-2.diff added

Rather use URL than email

comment:5 Changed 9 years ago by Malcolm Tredinnick

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 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(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