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 subschema titles to ErrorDetails #234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lalloni
Copy link

@lalloni lalloni commented Mar 28, 2019

Having such details on reported errors allows to use them
on localized error messages/templates for giving better error
context to caller/user.

As an example we're using this like this:

func (l locale) ConditionThen() string {
	return `Must validate "{{ if .thentitle }}{{ .thentitle }}{{ else }}then{{ end }}" as "{{ if .iftitle }}{{ .iftitle }}{{ else }}if{{ end }}" was valid`
}

func (l locale) NumberAllOf() string {
	return `Must validate all subschemas {{ if .subtitles }}{{ range .subtitles }}"{{ . }}"{{ end }}{{ else }}(allOf){{ end }}`
}

// etc...

I didn't include in the PR any modifications to DefaultLocale to preserve default behavior, but I could include that if requested.

Having such details on reported errors allows to use them
on localized error messages/templates for giving better error
context to caller/user.
@lalloni
Copy link
Author

lalloni commented Mar 29, 2019

This might help with #214

@johandorland
Copy link
Collaborator

First off thank you for your PR!
I'm looking at this with the discussion in #228 in the back of my head regarding errorMessage. If we're considering propagating fields like errorMessage, why not do the same for other fields like $comment, title and description? That'll give schema authors a lot more information on where an error came from.

Your PR takes quite a different approach and from the looks of it. I understand the need for extra information in the ErrorDetails() and using the title of the subschema is a decent way of doing it. The fundamental problem is that there is no API to retrieve nested errors and all the errors are just put in a linear array and the user has to interpret which errors belong together. With that in mind however until we make a new API (the upcoming draft-08 basically requires one with it's new outputting directives) stuffing ErrorDetails() with more information is the way to go.

I have a few points of feedback on your PR:

  • If title is not present it will just put in a nil value. If you just put in the title only when it's present users can write more idiomatic code instead of having to write nil checks.
  • The updated locale is a nice trick, but I don't think it should be done by default. As "title" is a core keyword it is used a lot in schemas. Users might not expect their errors to change whether a "title" is present or not. Not everyone using "title" in their schema will use it for the specific purpose you are using it for.
  • For the sake of consistency: If we add the titles of subschema for *Of and if/then/else, I think it's weird not to have the title of the main schema for all errors.
  • In my mind subtitles are part of movies. It might be a language thing, but I'd go for a less ambiguous term.

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