-
Notifications
You must be signed in to change notification settings - Fork 10
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 Date minimum/maximum constraints #57
Add Date minimum/maximum constraints #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor changes.
Furthermore, could you please move those changes to date.go and date_test.go? I've started this move with time{_test}.go but didn't finish.
Thanks!
schema/datetime.go
Outdated
func decodeDate(format, value string, c Constraints) (time.Time, error) { | ||
y, err := decodeDateWithoutChecks(format, value) | ||
if err != nil { | ||
return time.Now(), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please return v here? This was discussed and changed #54,
schema/datetime.go
Outdated
if c.Maximum != "" { | ||
max, err = decodeDateWithoutChecks(format, c.Maximum) | ||
if err != nil { | ||
return time.Now(), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same above. Please return max.
schema/datetime.go
Outdated
if c.Minimum != "" { | ||
min, err = decodeDateWithoutChecks(format, c.Minimum) | ||
if err != nil { | ||
return time.Now(), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same above, please return min, err
@danielfireman I'm following same patterns in the codebase, however, there are some things that I don't quite understand. Why is it that the test does assertions against private functions? Wouldn't be better to use |
Rename castDate to be decodeDateWithoutChecks to follow similar pattern as the decodeYearMonthWithoutChecks function.
f2ecb50
to
1d175fb
Compare
Hi @cored, thanks for your contributions! About your question:
As the code gets more complex, it is usually better to rely more heavily on unit testing. There are many reasons for that, including easier maintenance, faster, simpler (more benefits here). Let's say we decide to test that against the public Field.Decode. That simple test would involve: i) creating a field as a string, ii) call |
for issue #44