Skip to content

Fixing o2.Utilities.phpToMoment to support escaping #140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mariovalney wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixing o2.Utilities.phpToMoment to support escaping #140

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebc366c
Fixing o2.Utilities.phpToMoment to support escaping
May 11, 2017
e4efb78
Fix typo
mariovalney Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions js/utils/timestamp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ o2.Utilities.phpToMoment = function( s ) {
var m = '';
var lookBehind = '';
for ( var i = 0; i < s.length; i++ ) {
if ( lookBehind === "\\" ) { // Scaping character
Copy link
Contributor

@pablinos pablinos Aug 13, 2018

Choose a reason for hiding this comment

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

A nitpick but the comment should be 'Escaping character'

m += '[' + s.charAt( i ) + ']';
lookBehind = s.charAt( i );
Copy link
Contributor

@pablinos pablinos Aug 13, 2018

Choose a reason for hiding this comment

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

Do we want to do this? There are checks for sequences like jS, but if the j has been escaped then it may not be appropriate to trigger the substitution. Perhaps lookBehind should be empty?

continue;
}

switch ( s.charAt( i ) ) {
case 'd': // Day of the month with leading zeroes
m += 'DD';
Expand Down Expand Up @@ -106,6 +112,7 @@ o2.Utilities.phpToMoment = function( s ) {
case 'u': // Microseconds
case 'I': // Daylight savings time
case 'Z': // Timezone offset in seconds
case '\\': // Scape character
Copy link
Contributor

@pablinos pablinos Aug 13, 2018

Choose a reason for hiding this comment

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

Same nitpick comment as above :)

Copy link
Contributor

@pablinos pablinos Aug 13, 2018

Choose a reason for hiding this comment

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

Might there be a case for looking forward at this point and incrementing i? You'd have to be clever with the setting of lookBehind below, but it would keep the conditional logic all in the switch statement.

It's more something to think to check we're taking the right approach, than anything else.

break;

// Handle with lookBehind to handle 'jS'
Expand Down