Feature/descriptive validation messages #371
Conversation
validation.go
Outdated
| if key == "" { | ||
| keyFormatted = "Field" | ||
| } else { | ||
| var keyWords []string |
There was a problem hiding this comment.
It seems like you are using this as a slice of characters rather than slice of strings. A better type for that would be []rune
Also, this sort of routine is easy to extract into a function and unit testing -- do you mind writing a test for it?
…t for field formatting function
|
@robfig can you accept this PR? |
| @@ -243,6 +244,24 @@ func restoreValidationErrors(req *http.Request) ([]*ValidationError, error) { | |||
| return errors, err | |||
| } | |||
|
|
|||
| // Format field name for display | |||
| func formatFieldName(field string) (formattedField string) { | |||
| if field == "" { | |||
There was a problem hiding this comment.
It's more idiomatic to do checks and return early rather than nesting large blocks of code.
For example
if field == "" {
return "Field"
}
...There was a problem hiding this comment.
I see what you're saying, but what are you basing your measure of what's idiomatic for Go?
There was a problem hiding this comment.
What everyone on the golang-nuts mailing list says.
It's not the easiest thing to search for, but here's one reference:
http://talks.golang.org/2013/bestpractices.slide
There was a problem hiding this comment.
Alright, I'll get my code in line with the current best practices sometime this week, unless you'd rather scrap this PR for now in favor of reworking the validation system.
|
LG, just 1 comment. It would be nice if it was more flexible in general -- the previous messages were optimized for showing the error message inline next to the field (probably the UX-preferred way to handle form validation). These defaults are optimized for showing all messages at the top (Eng-preferred way). Too bad we can't have both! |
|
I'm sure we can have both, but maybe that's something to improve for Revel 2.0 |
|
@brendensoares are we fore going this for now? |
|
Ya, for now. I just backlogged it. |
jeevatkm
added
status-wrong-branch
type-enhancement
See #369