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

Auto-detect time format in form binding #1037

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alxarch
Copy link

@alxarch alxarch commented Jul 20, 2017

By using github.com/araddon/dateparse in binding/form_mapping.go the time_format tag can be optional.

@codecov
Copy link

codecov bot commented Jul 20, 2017

Codecov Report

Merging #1037 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1037   +/-   ##
=======================================
  Coverage   96.66%   96.66%           
=======================================
  Files          16       16           
  Lines        1711     1711           
=======================================
  Hits         1654     1654           
  Misses         49       49           
  Partials        8        8

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 7fafb3f...f4755ac. Read the comment docs.

@appleboy
Copy link
Member

Please add testing code.

@alxarch
Copy link
Author

alxarch commented Jul 20, 2017

There's no tests for form_mapping in general. Are they in a separate dir?

@alxarch
Copy link
Author

alxarch commented Jul 20, 2017

Ok I added a test to binding_test.go.

appleboy
appleboy previously approved these changes Jul 21, 2017
@appleboy
Copy link
Member

LGTM. @javierprovecho Please help to review.

@appleboy appleboy requested a review from javierprovecho July 21, 2017 02:36
@appleboy appleboy added this to the 1.3 milestone Jul 21, 2017
@andreynering
Copy link
Contributor

👍 for this change.

Do you really need to add another lib for this? We can try try some common formats like this:

var timeFormats = []string{
        "02/01/2006",
        "02/01/2006 15:04",
        "02/01/2006 15:04:05",
        // many other common formats
}

func tryParseTime(str string) (time.Time, error) {
        for _, fmt := range timeFormats {
                if t, err := time.ParseInLocation(fmt, str, time.Local); err == nil {
                        return t, nil
                }
        }
        return time.Time{}, ErrTimeFormatNotReconized
}


if val == "" {
value.Set(reflect.ValueOf(time.Time{}))
return nil
}

timeFormat := structField.Tag.Get("time_format")
if timeFormat == "" {
t, err := dateparse.ParseAny(val)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What location does ParseAny assume?

See below that the current behavior is using local timezone by default, unless a utc tag is informed in the field. I think it is desirable to keep this behavior.

Copy link
Author

@alxarch alxarch Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting from the araddon/dateparse module

The location your server is configured effects the results

There exists the dateparse.ParseIn(datestr string, loc *time.Location) (time.Time, error) method. I can switch to this to retain the tag behaviour.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the separate lib the only benefit is that it uses a byte reader with state switch to determine the format to be more efficient.

@alxarch
Copy link
Author

alxarch commented Jul 31, 2017

Changed the behaviour to respect time_utc tags

@firstrow
Copy link
Contributor

firstrow commented Aug 2, 2017

less 3rd party deps is better. I think user should be able to choose any datetime parsing lib he wants.

@alxarch
Copy link
Author

alxarch commented Aug 2, 2017

I don't see a way to add user choosable date parser without resorting to treating date args as strings and parsing dates in the controller. The whole point of binding is to get over with parsing request parameters with just adding a few tags to a struct. As to the separate dependency issue, the lib is quite simple and has it's own tests and documentation. Adding a simple for with predefined formats is a solution that can avoid using a dep but at the same time lays the burden of testing and documenting to the gin codebase. The current behavior is partly undocumented as it requires a time_format tag to function properly and only works for one format (and i don't see a way to add multiple formats because you can't distinguish
easily between a format separator and a time format character).

@javierprovecho javierprovecho modified the milestones: 1.x, 1.3 Aug 2, 2017
@javierprovecho
Copy link
Member

it's a feature, however i don't like the idea of loading chains of deps in gin, I've changed the milestone for the next version (not 1.4, already due), i'll give it a look later...

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

Successfully merging this pull request may close these issues.

5 participants