Open KamalGalrani opened 5 years ago
Merging #88 into master will decrease coverage by
2.88%
. The diff coverage is57.14%
.
@@ 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
Impacted Files | Coverage Δ | |
---|---|---|
parseany.go | 97.11% <57.14%> (-2.89%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 0fb0a47...21e4c6c. Read the comment docs.
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: Adding a second input to each function that accepts a spread of options
func ParseAny(datestr string, opts ...ParseOption) (time.Time, error) {...}
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