Opened 12 years ago

Closed 8 years ago

#4763 closed defect (invalid)

[CLA] [patch] dojo.substitute() returns undefined for unknown place holders

Reported by: wolfram Owned by: alex
Priority: high Milestone:
Component: Core Version: 1.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Currently dojo.string.substitute works like this:

    >>> dojo.string.substitute("${i} is not defined, but ${x} is", {x:"'this'"});
    undefined

but it should do this imho

    >>> dojo.string.substitute("${i} is not defined, but ${x} is", {x:"'this'"});
    ${i} is not defined, but 'this' is

My usecase: I am doing replacement in multiple steps and want to replace only when the right data are given.

Attachments (2)

string_substitute.patch (3.1 KB) - added by wolfram 12 years ago.
additional_tests_for_substitute_error_case.diff (1.0 KB) - added by wolfram 12 years ago.
This patch adds tests for dojo.string.substitue() that show the current misbehaviour

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by wolfram

Attachment: string_substitute.patch added

comment:1 Changed 12 years ago by Adam Peller

this conflicts with the stated behavior that dojo.string.substitutes throws if a match fails. This was done deliberately to flag cases where strings and code are mismatched that might otherwise go unnoticed by the developer. We can revisit that decision, but I don't think we can have it both ways (we don't want another flag here, do we?)

comment:2 Changed 12 years ago by wolfram

what do you mean by "both ways" does that not require a flag? Using the patch i provided would make it at least work like the String.replace method works ... just another thought

comment:3 Changed 12 years ago by wolfram

and its currently not throwing an exception or anything, it just returns undefined, which imho feels a bit odd

comment:4 Changed 12 years ago by Adam Peller

it throws for me, in trunk at least. What browser are you on?

dojo.string.substitute("${i} is not defined, but ${x} is", {x:"'this'"});

TypeError?: value has no properties

If a missing param doesn't throw, it's currently a bug.

The documented behavior was intentional, and different from the generic string.replace. The theory was that most of the time, it's a one-shot deal, and it's a very common error to miss a substitution or misspell something, so it's useful to flag a mismatch of data and code (IMO)

In your use case, is it possible/reasonable to accumulate the hash using a mixin or beget pattern and process it when you're ready?

Changed 12 years ago by wolfram

This patch adds tests for dojo.string.substitue() that show the current misbehaviour

comment:5 in reply to:  4 Changed 12 years ago by wolfram

Version: 0.91.0

Replying to peller:

it throws for me, in trunk at least. What browser are you on?

dojo.string.substitute("${i} is not defined, but ${x} is", {x:"'this'"});

TypeError?: value has no properties

If a missing param doesn't throw, it's currently a bug.

The documented behavior was intentional, and different from the generic string.replace. The theory was that most of the time, it's a one-shot deal, and it's a very common error to miss a substitution or misspell something, so it's useful to flag a mismatch of data and code (IMO)

In your use case, is it possible/reasonable to accumulate the hash using a mixin or beget pattern and process it when you're ready?

try the patch i just added, it fails for me. and the fault is the two additional test cases i added. can you confirm that?

comment:6 Changed 12 years ago by Adam Peller

Resolution: invalid
Status: newclosed

You're right, there should be a test case for this. I will incorporate your patch, thanks.

The assertError case works fine. However, if you call assertFalse on code that throws an exception, the exception will throw through the assert and doh will will report an error. In other words, assertFalse assumes a false result without an error, so I don't think the first test case is valid.

comment:7 Changed 12 years ago by Adam Peller

(In [11128]) Test for unmatched substitute. Refs #4763

comment:8 Changed 8 years ago by liucougar

Resolution: invalidfixed

In [28601]:

fixes #4763: modified various widgets to use dojo/mouse.wheel

comment:9 Changed 8 years ago by bill

Description: modified (diff)
Resolution: fixed
Status: closedreopened

Actually above checkin was for #3763.

comment:10 Changed 8 years ago by bill

Resolution: invalid
Status: reopenedclosed
Note: See TracTickets for help on using tickets.