feat(dateparser): Allow overriding of parsers. #6373
Conversation
| expect(dateParser.parse('31', 'yy').getFullYear()).toEqual(1931); | ||
| expect(dateParser.parse('30', 'yy').getFullYear()).toEqual(2030); | ||
| } | ||
| finally { |
There was a problem hiding this comment.
I don't think this is necessary to wrap in a try-finally - this factory gets reinstantiated with each test execution due to the whole beforeEach running of module('ui.bootstrap.dateparser')
There was a problem hiding this comment.
oh cool, didn't realize that. I'll pull that out then.
| var original = dateParser.getParser('yy'); | ||
| try { | ||
| expect(dateParser.parse('67', 'yy').getFullYear()).toEqual(2067); | ||
| expect(dateParser.parse('68', 'yy').getFullYear()).toEqual(2068); |
There was a problem hiding this comment.
These two lines probably aren't necessary, as they should already be covered by tests for the format
There was a problem hiding this comment.
One reason I had these here was because the dateParser appears to cache a parser instance for a particular format once it's already been used.
|
This mostly LGTM, just needs a little cleanup. |
|
@wesleycho how's that? I split the original test out into a few to better articulate the intent. |
|
I think the test cases can be simplified - what is mainly of concern is whether you can grab the current parser (and thus the original one), and whether you can override the parser used. That way if a failure in the overriding is present, it is easily isolated by the tests. |
|
So basically remove the "clears cached parsers when overridden" and we're good as it is implied in "can set a parser back to the original"? |
|
The two specs should read |
|
Ok, simplified. I still think it's important to have a test for making sure that the parsers are cleared when a parser is overridden. Basically guarding against this line (https://github.com/dougludlow/bootstrap/blob/b02815b446ad03df6eeb8a7ac054b13d9669f579/src/dateparser/dateparser.js#L258) being removed. If they aren't cleared the overridden parser will not be used. |
|
@wesleycho any other feedback here? |
|
LGTM, going to schedule for 2.4.0 |
Proof of concept for #6370.