Skip to content

Multiple errors for records? #4

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Multiple errors for records? #4

wants to merge 1 commit into from

Conversation

f-f
Copy link
Contributor

@f-f f-f commented Jun 4, 2024

This PR is not for merging - I am working on purescript/spago#1090, and noticed that since we have a list of errors when decoding, it feels like records should parse "in parallel", i.e. the code should try to parse all the keys and accumulate errors.

It currently doesn't, and bails out as soon as something errors (as demonstrated by the test, which should complain about n and m), and I guess this is a consequence of defining record sequentially with recordProp - @garyb do you see any way we could get multiple errors when decoding records?

@garyb
Copy link
Owner

garyb commented Jun 4, 2024

I gave this some thought but nothing easy is springing to mind. Assuming the recordProp codec was reworked to not just bind on decoding codecR and instead the errors were captured and combined, I don't think the error type combines things in a helpful way there, so yeah the sequential nature is a problem.

I did try figuring out a more "parallel" formulation for object/prop codecs when putting this version of the library together, but I couldn't come up with anything that worked out. I even tried dropping the Codec underpinning but that made other things worse. I feel like it should be possible though.

@f-f
Copy link
Contributor Author

f-f commented Jun 7, 2024

Something that I think is related is how the paths in a nested record error are appended. I printed one of the failures from Spago:

 {
  "causes": [
    {
      "causes": [
        {
          "causes": [],
          "message": "No value found",
          "path": "$.package.publish.license"
        }
      ],
      "message": "Could not decode PublishConfig",
      "path": "$.package.publish.license"
    }
  ],
  "message": "Could not decode PackageConfig",
  "path": "$.package.publish.license"
}

It's strange to me that the path in the outermost error has the same path of the innermost error, I'd expect them to look like this:

 {
  "causes": [
    {
      "causes": [
        {
          "causes": [],
          "message": "No value found",
          "path": "$.package.publish.license"
        }
      ],
      "message": "Could not decode PublishConfig",
      "path": "$.package.publish"
    }
  ],
  "message": "Could not decode PackageConfig",
  "path": "$.package"
}

I couldn't find a way to change withPath to do this though, presumably because this is all top-down.
In any case, this is no big deal in a world where decoding a record yields at most one error - though once you have different keys produce different errors, then lifting the path of the innermost error all the way up doesn't make sense anymore.

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