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

Meta issue for discussion of a debug API #4

Open
leafpetersen opened this issue May 16, 2018 · 5 comments
Open

Meta issue for discussion of a debug API #4

leafpetersen opened this issue May 16, 2018 · 5 comments

Comments

@leafpetersen
Copy link
Owner

Should there be a separate debug version of the classes? I deliberately avoided keeping a full trace of the path through the tree to the current parse point, since that requires allocation and/or putting handlers at every step. For performance, we should probably avoid that, at least in production. We could do nasty things with asserts, but it might be better to provide a separate version of the library cast_debug.dart that extends each class and intercepts the _cast calls to keep a stack. Is this necessary, or can we give good enough error messages without this?

@natebosch
Copy link

We definitely need to be able to give actionable errors to users if this data is coming from some configuration file and they wrote an incorrect type.

@leafpetersen
Copy link
Owner Author

Right - a question is, what level of detail is actionable? For example, given:

  var json = <String, dynamic>{
    "id": 0,
    "fields": <String, dynamic>{"field1": 1, "field2": 2},
    "extra1": "hello",
    "extra2": <dynamic>["hello"],
  };

    const typed = cast.Keyed<String, dynamic>({
      "id": cast.int,
      "fields": cast.Keyed({"field1": cast.int, "field2": cast.String}),
      "extra1": cast.OneOf(cast.String, cast.List(cast.String)),
      "extra2": cast.OneOf(cast.String, cast.List(cast.String)),
    });

Is this actionable enough?

  Failed cast at map entry field2: 2 is not a String
  package:cast/src/cast.dart 47:13               StringCast._cast
  package:cast/src/cast.dart 119:29              Keyed._cast
  package:cast/src/cast.dart 119:29              Keyed._cast
  package:cast/src/cast.dart 23:27               Cast.cast
  test/json_test.dart 57:11                      main.<fn>

Or do we need something more like:

Failed cast, "2 is not a String":
  at map entry for key "field2" 
  at map entry for key "fields"

The latter requires keeping a stack of the keys encountered along the path to the current cast point.

Not hard to do, and we could easily add a debug version that you could just swap in by importing cast_debug.dart, but I'm would be a little hesitant to put this on the fast path (especially for parsing).

@natebosch
Copy link

I suspect we need the latter. To get to parity with what we're able to do with package:yaml and hardcoded casts I think we'd need something even more complex since we can currently point to the line in the source file with an issue.

Hopefully with a path through keys (and indexes) we can figure out a way to get the source span from package:yaml

@leafpetersen
Copy link
Owner Author

For that though, I think you're not casting, you're parsing. That's what I'd like to get to. I'd like to either have (as part of the package) a parseYaml method on Cast, or have yaml subclass this in some way to provide it's own parsing. So then you don't do: schema.cast(parseYaml(string)), but rather schema.parseYaml(string) and your error messages come out in terms of source positions.

@natebosch
Copy link

Ah I see. The distinction between "parse" and "cast" makes sense to me. I don't have strong opinions on whether or when we need the extra context for "cast"

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

No branches or pull requests

2 participants