Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(dateParser): Add support for HH, H, mm, m, ss, s formats #3417

Closed
wants to merge 1 commit into from

Conversation

wesleycho
Copy link
Contributor

  • Add support for HH, H, mm, m, ss, s formats from Angular's dateFilter
  • Add support for : character in format expression

Working Plunker

This addresses #3159.

@@ -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, '\\:');
Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Contributor

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

@chrisirhc
Copy link
Contributor

Did a review. The main issue I see is the fix for : may not be adequate.

@wesleycho
Copy link
Contributor Author

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, '\\$&');
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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\:mmas 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.

Copy link
Contributor Author

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.

@chrisirhc
Copy link
Contributor

Did another review

@wesleycho
Copy link
Contributor Author

Linking related issues for reference #2508, #2509, #3159

- 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants