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

Add support for time.Duration type #642

Merged
merged 10 commits into from Sep 27, 2020
Merged

Add support for time.Duration type #642

merged 10 commits into from Sep 27, 2020

Conversation

elias19r
Copy link
Contributor

Fixes Or Enhances #584

Make sure that you've checked the boxes below before you submit PR:

  • Tests exist or have been written that cover this particular change.

Change Details:

Validations for time.Duration

Tag Coded in function Tested in function
eq= isEq() TestIsEqValidation()
ne= isNe() (not changed) TestIsNeValidation()
lt= isLt() TestIsLt()
lte= isLte() TestIsLte()
gt= isGt() TestIsGt()
gte= isGte() TestIsGte()
min= hasMinOf() (not changed) TestMinValidation() (new), TestMinMaxValidation() (new)
max= hasMaxOf() (not changed) TestMaxValidation() (new), TestMinMaxValidation() (new)
len= hasLengthOf() TestLenValidation() (new)

Cross-Field Validations for time.Duration

Tag Coded in function Tested in function
eqfield= isEqField() (not changed) TestIsEqFieldValidation()
nefield= isNeField() (not changed) TestIsNeFieldValidation()
ltfield= isLtField() (not changed) TestLtField()
ltefield= isLteField() (not changed) TestLteField()
gtfield= isGtField() (not changed) TestGtField()
gtefield= isGteField() (not changed) TestGteField()

Cross-Field Validations for time.Duration (within a separate struct)

Tag Coded in function Tested in function
eqcsfield= isEqCrossStructField() (not changed) TestCrossStructEqFieldValidation()
necsfield= isNeCrossStructField() (not changed) TestCrossStructNeFieldValidation()
ltcsfield= isLtCrossStructField() (not changed) TestCrossStructLtFieldValidation()
ltecsfield= isLteCrossStructField() (not changed) TestCrossStructLteFieldValidation()
gtcsfield= isGtCrossStructField() (not changed) TestCrossStructGtFieldValidation()
gtecsfield= isGteCrossStructField() (not changed) TestCrossStructGteFieldValidation()

Special

Tag Coded in function Tested in function
omitempty (not changed) It has been tested in all the previous test functions listed above
required hasValue() (not changed) I don't know how to test it, or if it is needed
isdefault isDefault() (not changed) I don't know how to test it, or if it is needed

Docs

Updated in the doc.go file.

@go-playground/admins

@deankarn deankarn merged commit 76981cc into go-playground:master Sep 27, 2020
@islishude
Copy link

Hi,It breaks our code!

type Type struct {
	Field        time.Duration `yaml:"field" validate:"required,min=5,max=300"`
}
field: 5

And get this

panic: time: missing unit in duration "5"

@elias19r
Copy link
Contributor Author

Thank you for letting us know! I'm sorry to hear that

To maintain backward compatibility, should we handle this err for "missing unit in duration" and assume nanosecond as default?

validator/util.go

Lines 233 to 240 in d6b17fd

// asIntFromTimeDuration parses param as time.Duration and returns it as int64
// or panics on error.
func asIntFromTimeDuration(param string) int64 {
d, err := time.ParseDuration(param)
panicIf(err)
return int64(d)
}

What do you think? @deankarn @islishude

@islishude
Copy link

I suggest reverting this commit.

my code is the same as below

var f time.Duation // parse from config file,it's just int64 here
var interval = f * time.Second // no type conversion is required here

@deankarn
Copy link
Contributor

Thanks for reporting @islishude

I'll try and take a look at this tonight after work, I think we just have to handle parsing of the param as an integer value also which will fix this.

@deankarn
Copy link
Contributor

@islishude @elias19r This has been fixed in release 10.4.1

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.

None yet

3 participants