Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

KamalGalrani
Copy link

fixes #28

yet to write tests

@codecov-io
Copy link

codecov-io commented Jul 9, 2019

Codecov Report

Merging #88 into master will decrease coverage by 2.88%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            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%) ⬇️

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.

@troyspencer
Copy link
Contributor

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

@KamalGalrani
Copy link
Author

KamalGalrani commented Aug 7, 2019 via email

@troyspencer
Copy link
Contributor

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).

@KamalGalrani
Copy link
Author

KamalGalrani commented Aug 7, 2019 via email

@troyspencer
Copy link
Contributor

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.

@troyspencer
Copy link
Contributor

I've made the PR for the functionality described above at #89

@KamalGalrani
Copy link
Author

KamalGalrani commented Aug 13, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Support for DD/MM/YYYY
3 participants