Skip to content
This repository has been archived by the owner on Apr 11, 2018. It is now read-only.

Add support for escaping characters with the date filter #432

Merged
merged 2 commits into from
Mar 7, 2014
Merged

Conversation

ecaron
Copy link
Contributor

@ecaron ecaron commented Mar 5, 2014

This PR adds character escape support, making the date filter closer to "PHP-style".

PHP usage being modelled is shown at http://us3.php.net/manual/en/function.date.php#example-2306

Per my and @paularmstrong preference, this PR supports one-escape-needed-per-character. As Paul says:

One escape per character makes more sense to me, as there doesn't seem to be a way to un-escape without a space.

@ecaron
Copy link
Contributor Author

ecaron commented Mar 5, 2014

If this gets accepted, will it be included in a 1.3.x release, or necessitate a 1.4.0 release?

{ c: 'v|date("S")', v: makeDate(420, 2011, 8, 23), e: 'rd' },

// Escape character
{ c: 'v|date("\\D")', v: d, e: 'D' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The double-slashes are necessary here to keep the TokenParser happy when performing the tests, and are not necessary in typical use. These tests are modelled after the test styling in the addslashes checks.

@paularmstrong
Copy link
Owner

@ecaron: I'm not sure. I'll have to review the other changes as well. Probably stay on 1.3.x, I think...

// Escape character
{ c: 'v|date("\\D")', v: d, e: 'D' },
{ c: 'v|date("\\t\\e\\s\\t")', v: d, e: 'test' },
{ c: 'v|date("\\\\D")', v: d, e: '\\Tue' }
Copy link
Owner

Choose a reason for hiding this comment

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

I attempted to add a more "real-world" example, and it failed tests...

      { c: 'v|date("jS \o\f F")', v: makeDate(420, 2012, 5, 4), e: '4th of July' }

@ecaron: Do you want to fix?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I may have done this wrong... need the extra escapes. Will re-try later tonight...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just going to say that - you're right that they need extra escapes because the first round of slashes gets lost converting the tests to code (just like in the addslashes example.)

@paularmstrong paularmstrong merged commit 0e71148 into paularmstrong:master Mar 7, 2014
@yocontra
Copy link

Any chance of a publish on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants