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

feature request: add error return to ValidateAny() #156

Open
mildwonkey opened this issue May 17, 2023 · 1 comment
Open

feature request: add error return to ValidateAny() #156

mildwonkey opened this issue May 17, 2023 · 1 comment
Labels
prio/mid Medium priority readiness/minimum Requisite for achieving a minimum readiness

Comments

@mildwonkey
Copy link

ValidateAny returns a nil instance if no conforming schema is found but no error, so you end up with this perfectly-valid-but-not-very-go-ish code block:

i := lin.ValidateAny(dashdata)
if i == nil {
  fmt.Errorf("BADONG") //  if you get this ref we are friends
}

It would be nice to have something more idiomatic:

i, err := lin.ValidateAny(dashdata)
if err != nil {
  return err
}

I think this would be nice to have even if we don't do anything to polish the returned errors (yet 😉)

@sdboyer
Copy link
Contributor

sdboyer commented May 18, 2023

i had a similar thought when i initially wrote it. But i also had a conundrum: in a multi-schema lineage where all schemas fail validation, which error do we return? In what order? For that matter, in what order should ValidateAny() move through all the schemas in a lineage?

Now that we're further along and starting to think more properly about errors, i don't actually find the above questions as intimidating: some kind of multierror that contains all of the schemas' error responses, keyed by version. (If we see other use cases for "error across all schemas," we could later expose a type for this.) And then yes, put that all into an additional error return value.

That said, ValidateAny() has always been a bit of a hack. It's fully implementable externally, so is redundant bloat on the Lineage interface. It was a shortcut that let me create vmuxers without having to design some more proper indirection patterns to, at minimum, allow the caller control over iteration order.

So: i'm 👍 to this change and would happily accept a PR, but would like to see it moved into a standalone func. An initial PR could kick the can down the road on iteration order - adding an error return and moving the func is sufficient. i suspect we can use a variadic option pattern later to avoid another breaking change.

@joanlopez joanlopez added prio/mid Medium priority readiness/minimum Requisite for achieving a minimum readiness labels Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio/mid Medium priority readiness/minimum Requisite for achieving a minimum readiness
Projects
None yet
Development

No branches or pull requests

3 participants