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

[kaidjohnson] Fix up dateparser support for 12 hour datetime format. #4117

Closed
wants to merge 1 commit into from
Closed

[kaidjohnson] Fix up dateparser support for 12 hour datetime format. #4117

wants to merge 1 commit into from

Conversation

kaidjohnson
Copy link

Using a date format that includes a 12-hour time format (eg. dd MMMM yyyy hh:mma) can result in a permanent invalid parsing of the value.

I've created a plunk to demonstrate the issue (try manually deleting the 'M' from 'PM' then putting the 'M' back. It becomes invalid without the M but never is valid again when the M is restored.):
http://plnkr.co/edit/CvJgnoCE08XGlDbVQka8?p=preview

Here's the same plunkr, but with the fix found in this PR:
http://plnkr.co/edit/uTKEjRFcY3CsWDHyZ0bZ?p=preview

@icfantv
Copy link
Contributor

icfantv commented Aug 6, 2015

@kaidjohnson please add the relevant functional test(s) for this and update/rebase/squash your PR. thanks.

'H': {
regex: '1?[0-9]|2[0-3]',
apply: function(value) { this.hours = +value; }
},
'mm': {
regex: '[0-5][0-9]',
regex: '[0-5]{1}[0-9]{1}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this change - this is no different than the previous version, as tested here.

@wesleycho wesleycho added this to the Backlog milestone Aug 6, 2015
@kaidjohnson
Copy link
Author

Thanks @icfantv and @wesleycho -- I've added tests, removed redundant {1}, and corrected AM/PM parsing issue discovered during tests. Also updated plunkr

@wesleycho
Copy link
Contributor

LGTM - good work!

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.

4 participants