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 more context to error messages #16

Open
joeldrapper opened this issue Jul 6, 2023 · 9 comments · May be fixed by #104
Open

Add more context to error messages #16

joeldrapper opened this issue Jul 6, 2023 · 9 comments · May be fixed by #104
Assignees
Milestone

Comments

@joeldrapper
Copy link
Owner

We can usually add some additional context such as the name of the attribute. Additionally, collection types could implement an alternative interface that allows them to highlight specifically which member failed and why.

@joeldrapper joeldrapper self-assigned this Sep 11, 2023
@joeldrapper joeldrapper added this to the 1.0 milestone Jun 23, 2024
@joeldrapper
Copy link
Owner Author

joeldrapper commented Jun 24, 2024

I’m struggling to find a nice way to present all the information. When we have a type failure, we’ll have three things:

  1. The type it was expected to be — this is usually inspectable, it’ll look like _Array(Integer) or something like that.
  2. The context (new) — this is something we can provide when asking Literal to check a type. It could be something like Class#method:property, but if that property is enumerated as part of the check (e.g. for an array or hash), it might have further context like Class#method:property[key][0]
  3. The value that was given that didn’t match the expected type. This is sometimes inspectable but often not really very inspectable. It could be like #<Foo:0x0000000100fe3660>......... When presenting the value, it might make sense to try to get its class (if it’s an object) and mention that first, e.g.
Received a String: `"actual string"`.
  1. The final bit of information is where the error occurred. We should be able to highlight the exact line number where the method was called and given the wrong type of argument.

@hangsu
Copy link

hangsu commented Jun 24, 2024

Just wanted to say that (2) would be a great addition.

Given that our codebase is made up of almost entirely Literal::Attributes and Literal::Datas, a specific type violation can be hard to track down.

Presentation-wise, I think RSpec output does a decent job of presenting an expected vs actual value, and I see this as somewhat analogous.

Looking forward to 1.0! We're still pinned to 455f225574dbcb39b7e325a9f75c928a79ec812b and are eager to upgrade/simplify.

@joeldrapper
Copy link
Owner Author

Looking forward to 1.0! We're still pinned to 455f225 and are eager to upgrade/simplify.

We’re really close now.

@joeldrapper
Copy link
Owner Author

Here’s where I’m at so far.

CleanShot 2024-06-25 at 19 02 47@2x

@hangsu
Copy link

hangsu commented Jun 25, 2024

Maybe:

Person#initialize
  Type mismatch for prop `name`
    Expected: `String`
    Actual: `Integer(1)`

Here's Sorbet's type error: https://sorbet.org/docs/error-reference#7002

Edit: prop instead of argument

@joeldrapper
Copy link
Owner Author

Made some progress on this in #98, but we can go further. Specifically, I would like to make collection types recursively add further context, so you know which specific key a check failed on.

@Vagab
Copy link

Vagab commented Aug 15, 2024

hey @joeldrapper. Is there anything I can do to help? Otherwise, do you know if that's the last thing that will change until literal 1.0? In other words is it valid to use main right now?

@joeldrapper
Copy link
Owner Author

I don’t expect to ship any significant changes to main. I think this is the last bit.

@Vagab
Copy link

Vagab commented Aug 27, 2024

ok thank you! Is there anything I can do to help with this issue btw?

@joeldrapper joeldrapper linked a pull request Sep 13, 2024 that will close this issue
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 a pull request may close this issue.

3 participants