Fixing o2.Utilities.phpToMoment to support escaping#140
Fixing o2.Utilities.phpToMoment to support escaping#140mariovalney wants to merge 2 commits intoAutomattic:masterfrom mariovalney:issue-135
Conversation
|
Is this still valid? Some feedback would be appreciated. :) |
pablinos
left a comment
pablinos
left a comment
There was a problem hiding this comment.
I spotted this PR because I was working on something else. I realise it was submitted a while ago, but thanks for taking the time to work on this!
I've added a few comments that I hope are helpful. It would also be really useful to write a few tests to cover these cases.
Thanks again!
js/utils/timestamp.js
Outdated
| var m = ''; | ||
| var lookBehind = ''; | ||
| for ( var i = 0; i < s.length; i++ ) { | ||
| if ( lookBehind === "\\" ) { // Scaping character |
There was a problem hiding this comment.
A nitpick but the comment should be 'Escaping character'
| for ( var i = 0; i < s.length; i++ ) { | ||
| if ( lookBehind === "\\" ) { // Scaping character | ||
| m += '[' + s.charAt( i ) + ']'; | ||
| lookBehind = s.charAt( i ); |
There was a problem hiding this comment.
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?
js/utils/timestamp.js
Outdated
| case 'u': // Microseconds | ||
| case 'I': // Daylight savings time | ||
| case 'Z': // Timezone offset in seconds | ||
| case '\\': // Scape character |
There was a problem hiding this comment.
Same nitpick comment as above :)
js/utils/timestamp.js
Outdated
| case 'u': // Microseconds | ||
| case 'I': // Daylight savings time | ||
| case 'Z': // Timezone offset in seconds | ||
| case '\\': // Scape character |
There was a problem hiding this comment.
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.
3 participants
o2.Utilities.phpToMoment should support escape some chars like in pt_BR and pt_PT dates.
For example,
j \d\e F \d\e Yshould returnD[ ][d][e][ ]MMMM[ ][d][e][ ]YYYY.Issue #135