-
Notifications
You must be signed in to change notification settings - Fork 6.7k
feat(dateParser): Add support for HH, H, mm, m, ss, s formats #3417
Conversation
b5bb2eb
to
342a0ca
Compare
@@ -81,7 +105,7 @@ angular.module('ui.bootstrap.dateparser', []) | |||
return input; | |||
} | |||
|
|||
format = $locale.DATETIME_FORMATS[format] || format; | |||
format = $locale.DATETIME_FORMATS[format] || format.replace(/:/g, '\\:'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tests will fail if a DATETIME_FORMAT
from $locale
containing :
is used.
Should probably add some of those formats in the dateparser.spec below (e.g. a format of short
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do a replace on the next line instead then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better fix might be to escape all special characters in the regex before doing new RegExp
on the createParser
function.
d3.requote
has a nice regexp to do this. See: https://github.com/mbostock/d3/blob/master/src/format/requote.js
Did a review. The main issue I see is the fix for |
342a0ca
to
c3694be
Compare
Made the change requested - can you re-review this? |
@@ -82,6 +108,7 @@ angular.module('ui.bootstrap.dateparser', []) | |||
} | |||
|
|||
format = $locale.DATETIME_FORMATS[format] || format; | |||
format = format.replace(SPECIAL_CHARACTERS_REGEXP, '\\$&'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should instead do this right before the new RegExp()
call inside createParser
as that shows why this character replacement is needed (it's to be fed into the RegExp constructor).
There's also less weird characters to process in the createParser
's formatCodeToRegex
loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the less weird characters part? Just moving this inside the createParser
function isn't enough, there seems to be some specificity required as to when the parsing happens since the regex gets some additional characters through the iteration over formatCodeToRegex
. The loop adds an open and closed set of parentheses to the regex. This means that it would be too late to modify the format string at that stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yea, I'm referring to replacing the characters right before the parentheses are added.
With the less weird characters, I'm referring to that if given a format of hh:mm
, the createParser
will now receive hh\:mm
as the format when you do this here. It doesn't need that extra \
when looking for hh
or mm
(in the angular.forEach(formatCodeToRegex…
block), it only needs the extra \
right before creating the regex
property when feeding it into the RegExp
constructor with the parentheses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per discussion between @chrisirhc and I on Slack, this turns out to be the best spot for modifying the format given how the iteration in formatCodeToRegex
is implemented.
Did another review |
e049bbb
to
b19af65
Compare
- Add support for `HH`, `H`, `mm`, `m`, `ss`, `s` formats from Angular's `dateFilter` - Add support for `:` character in format expression - Fix typos - Add regexp escaping of special characters
b19af65
to
d466c9b
Compare
HH
,H
,mm
,m
,ss
,s
formats from Angular'sdateFilter
:
character in format expressionWorking Plunker
This addresses #3159.