-
Notifications
You must be signed in to change notification settings - Fork 356
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
Return error from Isformat() and include in DoesNotMatchFormatError ErrorDetails #300
base: master
Are you sure you want to change the base?
Conversation
Now -run is more useful and the logs look like this: --- PASS: TestFormats (0.06s) --- PASS: TestFormats/draft4 (0.01s) --- PASS: TestFormats/draft4/validation_of_date-time_strings (0.00s) --- PASS: TestFormats/draft4/validation_of_date-time_strings/a_valid_date-time_string (0.00s) --- PASS: TestFormats/draft4/validation_of_date-time_strings/an_invalid_date-time_string (0.00s) --- PASS: TestFormats/draft4/validation_of_date-time_strings/only_RFC3339_not_all_of_ISO_8601_are_valid (0.00s) --- PASS: TestFormats/draft4/validation_of_URIs (0.00s) --- PASS: TestFormats/draft4/validation_of_URIs/a_valid_URL_with_anchor_tag (0.00s) --- PASS: TestFormats/draft4/validation_of_URIs/a_valid_URL_with_anchor_tag_and_parantheses (0.00s)
Update README.md err.Type() to include "format" (and sort the list). Add comment on ErrBadFormat. Update comment on FormatChecker IsFormat.
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.
Nice work @timbunce ! Man, it's so much simpler if we just make things not backwards compatible lol. Way less code change and more user-friendly than what I suggested.
Obviously, still wait on @johandorland 's input.
if err := FormatCheckers.IsFormat(currentSubSchema.format, value); err != nil { | ||
details := ErrorDetails{"format": currentSubSchema.format} | ||
if err != ErrBadFormat { | ||
details["error"] = 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.
NIT but can we potentially change error
to fmtError
? Right now, ErrorDetails
is overloaded with a lot of other key names like expected
, reference
, format
, etc. so error
becomes extremely unclear in what context this key should be used or overloaded.
Or maybe call it customMsg
or customError
if we want this custom error injection in other validation checks in the future.
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.
ErrorDetails holds "a map of details specific to each error". In this case it's a DoesNotMatchFormatError
type error.
I think the error
key fits in very well with the existing general key names like expected
, reference
, format
, etc. @johandorland would be better able to judge though.
In looking into this I noticed that I hadn't updated the relevant comment in errors.go, so I've done that now in 4a6c4b6.
…mment Also add `-` to another ErrorDetails comment to be consistent with others.
@johandorland The tests are failing due to maps being unordered in %v in Go v1.11. I could work around that if you're like, let me know, but it would be simpler to just drop Go 1.11 from the Travis config (and add 1.14). |
Ok, that's not a problem. It could be a while before I have the time to look through this thorougly, but don't worry, it's on the list. |
Anything I can do to help, @johandorland? |
Hey @johandorland, anything I can do to help? |
Hello, |
Perhaps best to review the commits individually. (Ignoring whitespace helps with the indentation change in fd8e6d1.)
The first three commits are backwards compatible:
The last few commits comprise the desired changes:
This is based on @Ferix9288's idea and work in #254.
I've just simplified it after @johandorland confirmed he was ok with tagging a v2.