Unify union types in the same error #17

gillchristian · 2019-12-22T22:11:08Z

Improves on #5

I started to work on the reporting of union types errors. It's not yet finished, but I'd like some feedback on the code (I think my editor applied automatic formatting btw 🤦‍♂) and also on how the errors look.

Here are some examples one the errors, and what's still missing.

t.keyof

io-ts suggests to use t.keyof for union types of only string literals.

const OneOf = t.interface({
    oneOf: t.keyof({ a: null, b: null, c: null }),
});

report(OneOf.decode({ oneOf: '' }));

That still reports as before:

Expecting "a" | "b" | "c" at oneOf but instead got: "A".

Literals unions (using t.union):

const StringUnion = t.interface({
    stringUnion: t.union([t.literal('a'), t.literal('b'), t.literal('c')]),
});
report(StringUnion.decode({ stringUnion: 'A' }));

Reports with the new format (instead of showing 3 different errors):

Expecting one of:
    "a"
    "b"
    "c"
at stringUnion but instead got: "A".

Things to improve:

There are some things that should still be improved. For unions of other types, I need to do some improvements on the usage of the context to get proper errors.

const Unions = t.interface({
    interfaceUnion: t.union([
        t.interface({ key: t.string }),
        t.interface({ code: t.number }),
    ]),
});

For such union, the following input would give the error one expects:

report(Unions.decode({ interfaceUnion: 'name' }));

Expecting one of:
    { key: string }
    { code: number }
at interfaceUnion but instead got: "name".

But for the case when there's more context of the value:

report(Unions.decode({ interfaceUnion: {} }));

The error is wrong 😕

Expecting one of:
    string
    number
at interfaceUnion but instead got: undefined.

gillchristian · 2019-12-22T22:14:54Z

As a side note, I think it would be beneficial to add assertions to the tests, since changes such as this PR would require several cases being checked.

Also I could add prettier (to prevent what happened with my changes).

Didn't want to go ahead and add such stuff since it's not my call 😅 But I'm open to make such additions if you think it's beneficial.

OliverJAsh · 2020-02-04T16:06:19Z

Hey @gillchristian, really sorry for taking so long to get back to you on this.

I'm not using io-ts at the moment and I'm finding it really hard to remember how everything fits together. It might be better if I just added you add a contributor to this repo, or you could maintain a fork (I'd be happy to link from my repo to yours)?

gillchristian · 2020-02-04T16:27:26Z

No worries. I parked the work on this PR from my side anyways 😴

If you are fine with having a contributor I'd prefer that since a fork would be misleading. I'm also happy to maintain the project in general, since it's already the one listed in fp-ts / io-ts websites better to keep this one alive, wdyt?

OliverJAsh · 2020-02-04T17:17:29Z

@gillchristian I've sent you an invite :-)

gillchristian · 2020-03-06T23:48:08Z

Finally put some time into this. The errors look decent now.

There could be some more work into making the formatting smart: eg. if the provided value is "closer" to the type of one of the branches of the union, then only report to that one. But for now is good enough I guess.

osdiab · 2020-05-20T05:58:22Z

I made a PR on your repo that merges in the current master and converts your tests to the new test format FYI!

osdiab · 2020-05-20T05:59:21Z

src/index.ts

+        : O.some(
+              // https://github.com/elm-lang/core/blob/18c9e84e975ed22649888bfad15d1efdb0128ab2/src/Native/Json.js#L199
+              // tslint:disable-next-line:prefer-template
+              `Expecting one of:\n${expected}` +


one thing I'd change with your PR is that now there's two different places where the same error message is being constructed, either find a way for both code paths to merge, or extract the error message code to its own helper

my use case is that in supporting #25 , this function now doesn't have a specific error object associated with it, so it doesn't make sense what error message to show if multiple are present in the union case - for now in my fork i'm just not showing errors if it's a union error for simplicity, but ideally if there's one "dominant" error like mentioned in #5 to make it choose a branch smartly, that one would probably be the error message I'd display.

that and having two places to construct the same message seems like a likely way for them to go out of sync and introduce regressions, etc.

if my error message PR doesn't go through and this reporter chooses not to show error messages, then this particular problem isn't important, though not suppressing error messages seems like a good thing to me

but ideally if there's one "dominant" error like mentioned in #5 to make it choose a branch smartly, that one would probably be the error message I'd display.

Totally agree on that.

And yes, we should unify the way error messages are generated.

osdiab · 2020-05-20T13:24:30Z

FYI: I haven't looked into the code to figure out why, but I tried this branch decoding a codec of mine with some garbage data, and got a lot of duplicate values for the union:

// codec looks roughly like this
export const orgData = t.intersection([
  t.type({
    // ... a bunch of other entries including several union types
    revenueAmt: t.union([NumberFromString.pipe(BoundedInt), t.null]) 
  }),
    t.type({ id: t.string }),
    t.partial({
      customerInfo: customerInfoCodec // also a fairly complex type
    })
])

Result

Expecting one of:
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
    pipe(NumberFromString, BoundedInt)
    null
at revenueAmt but instead got: "5.24".

gillchristian · 2020-05-20T20:38:32Z

@osdiab I think that is because it adds errors for every branch or something like that.

I need to get this PR in better shape, I'll take a look at your changes on the tests and pick up the work I was doing.

P.S I'll do some work on this and ping you

gillchristian · 2020-05-21T21:10:18Z

tests/index.test.ts

+        reporter(
+            Person.decode({
+                name: 'Jane',
+                gender: 'female',
+                children: [{ gender: 'Whatever' }]
+            })
+        ),
+        [
+            'Expecting number at age but instead got: undefined',
+            [
+                'Expecting one of:',
+                '    "Male"',
+                '    "Female"',
+                '    "Other"',
+                'at gender but instead got: "female"'
+            ].join('\n'),


@osdiab I think this case covers the error you pointed out on #17 (comment)

Expected:

Expecting one of: "Male" "Female" "Other" at gender but instead got: "female"

But now it outputs:

Expecting one of: "Male" "Female" "Other" "Male" "Female" "Other" at gender but instead got: "female"

Note that I only added the failing tests for now. I still need to work on the code.

osdiab · 2020-05-29T14:41:22Z

any way I can help move this along? I'd love to get this into my prod codebase :) I can take a look in a couple days and make some changes if you're too busy to sink time into it.

gillchristian · 2020-05-29T14:49:07Z

I'll do some progress on Sunday & Monday (holiday here) and push my progress in case I do not finish. Wdyt?

osdiab · 2020-05-29T16:25:38Z

works for me, no pressure! just excited to see it go in, it would make error logs at my company so much more readable haha

gillchristian · 2020-05-29T16:31:08Z

it would make error logs at my company so much more readable haha

Same reason why I started this PR :)

Improves on #5

gillchristian · 2020-06-01T19:20:29Z

@osdiab I guess it should be good to go know.

We certainly can iterate on improvements (eg. only show the error from the type of the union that is closer). But for now I think it's fine like this.

Please check the tests to see how errors are formatted in different cases

osdiab

test cases look good to me, and we can add more and refine iteratively :) thanks for the great PR!

osdiab · 2020-06-02T02:54:11Z

src/utils.ts

+import { Predicate } from 'fp-ts/lib/function';
+
+/* eslint-disable @typescript-eslint/array-type */
+export const takeUntil = <A = unknown>(predicate: Predicate<A>) => (


If implemented a find function this can be a one-liner btw, ES6 equivalent as.slice(0, as.findIndex(predicate) + 1) - but since find isn't available in all runtimes would need to implement it

Yes find could work as well.

I took a quick look at how takeWhile is implemented in fp-ts and decided to go with that approach.

hasparus · 2020-06-23T09:53:35Z

src/index.ts

 const jsToString = (value: t.mixed) =>
  value === undefined ? 'undefined' : JSON.stringify(value);

-export const formatValidationError = (error: t.ValidationError) => {


Ouch. This was a breaking change for me :D

Ohh sorry! I'll find a way to fix it today.

Now there's a version for union errors and "other" errors. Because errors on union types are basically treated as one error per case of the union type so what I did was group by the path of the property that had an error. I'll look into a way of generalizing that and if it's not possible I might have to export both functions.

Check #33

I also added another exported function that can be used to format the errors 'in place'

import * as t from 'io-ts'; import { formatValidationErrors } from 'io-ts-reporters'; import * as E from 'fp-ts/lib/Either'; import { pipe } from 'fp-ts/lib/pipeable'; const User = t.interface({ name: t.string }); pipe({ nam: 'Jane' }, User.decode, E.mapLeft(formatValidationErrors));

gillchristian requested a review from OliverJAsh March 6, 2020 23:48

gillchristian changed the title ~~[WIP] Unify union types in the same error~~ Unify union types in the same error Mar 7, 2020

gillchristian added the enhancement label Mar 7, 2020

gillchristian mentioned this pull request May 13, 2020

Hard to debug union types #5

Closed

osdiab reviewed May 20, 2020

View reviewed changes

gillchristian commented May 21, 2020

View reviewed changes

gillchristian added 2 commits June 1, 2020 21:01

feat(fmt): Unify union types in the same error

b68c241

Improves on #5

fix(linter): Warnings & errors

21e9f55

osdiab approved these changes Jun 2, 2020

View reviewed changes

osdiab reviewed Jun 2, 2020

View reviewed changes

gillchristian merged commit 9792b5a into gillchristian:master Jun 2, 2020

gillchristian deleted the feature/union-types-formatting branch June 2, 2020 07:51

hasparus reviewed Jun 23, 2020

View reviewed changes

gillchristian mentioned this pull request Jun 24, 2020

fix: Expose format errors functions #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify union types in the same error #17

Unify union types in the same error #17

gillchristian commented Dec 22, 2019 •

edited

gillchristian commented Dec 22, 2019

OliverJAsh commented Feb 4, 2020 •

edited

gillchristian commented Feb 4, 2020

OliverJAsh commented Feb 4, 2020

gillchristian commented Mar 6, 2020

osdiab commented May 20, 2020

osdiab May 20, 2020 •

edited

osdiab May 20, 2020

gillchristian May 20, 2020

osdiab commented May 20, 2020 •

edited

gillchristian commented May 20, 2020 •

edited

gillchristian May 21, 2020

osdiab commented May 29, 2020

gillchristian commented May 29, 2020

osdiab commented May 29, 2020

gillchristian commented May 29, 2020

gillchristian commented Jun 1, 2020

osdiab left a comment

osdiab Jun 2, 2020

gillchristian Jun 2, 2020

hasparus Jun 23, 2020

gillchristian Jun 23, 2020

gillchristian Jun 24, 2020

Unify union types in the same error #17

Unify union types in the same error #17

Conversation

gillchristian commented Dec 22, 2019 • edited

gillchristian commented Dec 22, 2019

OliverJAsh commented Feb 4, 2020 • edited

gillchristian commented Feb 4, 2020

OliverJAsh commented Feb 4, 2020

gillchristian commented Mar 6, 2020

osdiab commented May 20, 2020

osdiab May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osdiab commented May 20, 2020 • edited

Result

gillchristian commented May 20, 2020 • edited

Choose a reason for hiding this comment

osdiab commented May 29, 2020

gillchristian commented May 29, 2020

osdiab commented May 29, 2020

gillchristian commented May 29, 2020

gillchristian commented Jun 1, 2020

osdiab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gillchristian commented Dec 22, 2019 •

edited

OliverJAsh commented Feb 4, 2020 •

edited

osdiab May 20, 2020 •

edited

osdiab commented May 20, 2020 •

edited

gillchristian commented May 20, 2020 •

edited