-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exposes monthfirst #88
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
- Coverage 100% 97.11% -2.89%
==========================================
Files 1 1
Lines 899 935 +36
==========================================
+ Hits 899 908 +9
- Misses 0 23 +23
- Partials 0 4 +4
Continue to review full report at Codecov.
|
I wrote tests for the newly exposed preferMonthFirst input, and confirmed the functionality of it. However, I think that the macro level changes to the design should be rethought. By adding an additional required input to each function, it constitutes a breaking change which will mean a major version bump. Second, if additional options were to be exposed in a similar way, it wouldn't scale well. Finally, this PR conflates the exposition of preferMonthFirst with an additional retry option which was applied onto many of the functions as a wrapper that absorbs an error and retries with the opposite preference. This could serve as another option instead. So I propose the following design:
This model is demonstrated here |
Thanks for the feedback. Will try to update when I find time.
P. S.: My knowledge of golang is limited to the contents of the pull request
|
I have a good idea on how to translate the model for this package, work in Go daily, and have a project which would greatly benefit from these changes. I think I can get it done in the next few days. I can either PR to your branch, or make another PR to the master with the updated scope (with your changes credited). |
Thanks. I wouldn't mind either.
|
Pretty much finished with this today; the results are on my fork of this. I ended up just cherry picking the few functional changes onto the master, then applying the new macro design changes onto that. It adds PreferMonthFirst and RetryAmbiguousDateWithSwap as options mutating the private options. An example usage of these is actually done near the beginning of parseTime, in the implementation of the functionality of RetryAmbiguousDateWithSwap. The tests for complete code coverage should be pretty easy to hit due to the increased branches being minimal (new decision trees only occur within parseTime, newParser, and the new option functions). However, some more logical tests should probably be added as well. |
I've made the PR for the functionality described above at #89 |
👍🏻
|
fixes #28
yet to write tests