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

Improve default for durations #464

Merged
merged 1 commit into from
Jan 21, 2021
Merged

Conversation

riptl
Copy link
Contributor

@riptl riptl commented Jan 18, 2021

Summary: This improves support for the default struct tag on time.Duration.

Problem

time.Duration is an int64 type, so it's already supported by the default tag.
However, the default is interpreted as a number of nanoseconds, not a duration, making parsing a bit awkward.

Example for 120ms:

var doc struct {
    DurationField time.Duration `default:"120000000"`
}

Feature

Go's time library supports a human-readable notation for durations.
Including this improves readability:

var doc struct {
    DurationField time.Duration `default:"120ms"`
}

Implementation notes

To preserve backwards-compatibility, the code checks for three conditions:

  • The type is int64
  • The type name is time.Duration
  • The last char of the provided default is not a number

If either is not met, the code falls back to parsing a number.

Open questions:

  • Is the backwards-compatibility check overkill, or good means of preventing people from shooting themselves in the foot?
  • Instead of checking .Type().String(), is it preferred to check for .Type().PkgName() and .Type().Name() for a tiny bit more performance?

@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #464 (69117ae) into master (ba1b12b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   93.60%   93.62%   +0.02%     
==========================================
  Files          10       10              
  Lines        2266     2274       +8     
==========================================
+ Hits         2121     2129       +8     
  Misses        105      105              
  Partials       40       40              
Impacted Files Coverage Δ
marshal.go 95.91% <100.00%> (+0.05%) ⬆️

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 652b9f8...69117ae. Read the comment docs.

Copy link
Owner

@pelletier pelletier left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the patch!

@pelletier pelletier merged commit a713a3e into pelletier:master Jan 21, 2021
@riptl riptl deleted the default-duration branch January 23, 2021 01:26
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.

2 participants