search

angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.27k stars 6.73k forks source link

feat(dateparser): Allow overriding of parsers. #6373

Closed dougludlow closed 7 years ago

dougludlow commented 7 years ago

Proof of concept for #6370.

wesleycho commented 7 years ago

This mostly LGTM, just needs a little cleanup.

dougludlow commented 7 years ago

@wesleycho how's that? I split the original test out into a few to better articulate the intent.

wesleycho commented 7 years ago

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.

dougludlow commented 7 years ago

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"?

wesleycho commented 7 years ago

The two specs should read it('should get the current parser' And it('should override the parser' Or something like that, and test those scenarios

dougludlow commented 7 years ago

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.

dougludlow commented 7 years ago

@wesleycho any other feedback here?

wesleycho commented 7 years ago

LGTM, going to schedule for 2.4.0