Enforce callers pass a path for debugging purposes, and update ppx to…

[erlandsona]

… generate better location info for decode failures.

So this is just a first pass at upgrading decco for better location information on decode errors. I haven't update the tests yet because I'm not sure how to fix this issue where it seems like the Pervasives.__LOC__ calls are being evaluated when the ppx is run rather than printing that expression for bucklescript to compile.

If I someone can help me figure that part out then I can fix the tests and we all can go from this
{path: "[0].id", message: "Not a int", value: nil}
to something more along the lines of this...
{path: "[0].id File "MyCustomRecord.re", line 8, characters 7-60", message: "Not an int", value: nil}

This will also close #54

[erlandsona]

So I finished this version but I'd really like some input or help making this interface less invasive to calling code... It really stinks to 1 have a major version bump in the API and 2 to force callers to have to pass __LOC__ into all the codecs when building a custom decoder.

Generally I'd prefer if we could do more PPX magic and somehow generate the Location info wherever something like Decco.intFromJson is used.

Also, the formatter I have from bs-platform doesn't play nice with this so I tried generally not to touch any formatting stuff but if there's a tool I can run on the project to get this PR up to speed with a particular formatting ruleset I'd be happy to run that.

[erlandsona]

@ryb73 you have any thoughts on this?

[rickyvetter]

Can you share an example before/after error message? Would be interested to see how this looks for folks using decco.

[erlandsona]

@rickyvetter Of course!
If you take a look at the test files you can see a direct comparison of what the before/after messages look like.
https://github.com/reasonml-labs/decco/pull/55/files#diff-4887d5e04c82d3ad103e1500793d43e6
https://github.com/reasonml-labs/decco/pull/55/files#diff-a24b1337669f9c22a77d36a098c12a98

[rickyvetter]

Oh wow. I'm oblivious. Looks cool!

[erlandsona]

I'm not a fan of the breaking API change that'll force users writing "custom decoders" with the Codecs to pass an argument, it's better in that it'll force callers into a debuggable pattern but I wish I could code gen location info for failures directly to call sites of stuff...

[ryb73]

Thanks for the contribution @erlandsona.

I'm afraid I'm not really understanding the use case behind this. It's a pretty standard thing for functions that can fail to return options or results, and those return values generally don't include location information. If the caller wishes to log an error with location data, the caller can use __LOC__ or Js.Exn at that time. I don't really see the value of baking it into the library?

[erlandsona]

Two things...

1. When we use the decco annotation for a type without the location info if any of the decoders for properties on that type fail it's not always clear from the name of the property which type or which file is defining the property that's failing to decode.

2. For custom decoders unless there's a way to pull location information of the call site via ppx these changes force calling code to provide location information for themselves in the future.

My power just went out so I'm typing this up on my phone otherwise I'd have more complete examples to share here. But I'm hoping I'm making enough sense otherwise.

[ryb73]

Sorry about the power, hope you got it fixed!

For custom decoders unless there's a way to pull location information of the call site via ppx these changes force calling code to provide location information for themselves in the future.

Is it not sufficient to get the location information at the call site when it fails?

let a = [||];
switch (Js.Array.pop(a)) {
  | Some(v) => Js.log2("got value", v);
  | None => Js.log2("failure at", Pervasives.__LOC__);
}

// same concept applied to decco
switch (mytype_decode(json)) {
  | Ok(v) => Js.log2("got value", v);
  | Error(_) => Js.log2("failure at", Pervasives.__LOC__);
  // or: // | Error(_) => Js.Exn.raiseError("failure")
}

[erlandsona]

Alrighty I'm back at my machine... Powers on and all is right with the world 😆

RE: Is it not sufficient to get the location information at the call site when it fails?

It is sufficient to configure the location information at the call site when it fails but I don't believe that a special "Debugging" (compiler hook?) should be something the callers have to be aware of to benefit from.

I don't really see the value of baking it into the library?

Aside from my answer to the first point here's some code...

CodingQuery.re

module Record = {
  module Id = ID.Int();

  [@decco]
  type t = {
    id: Id.t,
    submission: string,
    submittedById: UserT.Id.t,
    submittedBy: UserT.record,
    submittedAt: Date.t,
    encounterId: EncounterT.Id.t,
    encounter: EncounterT.record,
    response: option(string),
    respondedById: option(UserT.Id.t),
    respondedBy: option(UserT.record),
    respondedAt: option(Date.t),
  };

  // consider using lenses-ppx to push this definition into Tabular.
  let pkey = x => x.id;
};

// Spits out a Data (Map.t) module and a View (Html Table Component) module.
module Table = Tabular.Make(Record);

module ResponseT = {
  type data = Async.t(StatusT.t);
  type action =
    | Change(Record.Id.t, string)
    | Send(Record.Id.t, EncounterT.Id.t, data);
};

type state = Async.t(Table.t);
type action =
  | Fetch(state)
  | Response(ResponseT.action);

let init = AsyncResult.init;
let reduce: (state, action) => state =
  Relude_Function.curry2(
    fun
    | (_, Fetch(a)) => a
    | (state, Response(a)) => state->Response.reduce(a),
  );

let encounterFetch = (id: EncounterT.Id.t) =>
  Request.make(
    ~creator=x => CodingQuery(Fetch(x)),
    API.get(
      "/coding_dashboard/queries/" ++ id->EncounterT.Id.show,
      ~decode=CodingQueryT.Table.t_decode,
    ),
  );

let appsList = (viewingAs: UserT.Id.t) =>
  Request.make(
    ... // Action creator, optional selector, optional toast message options.
    API.get(
      "/coding_dashboard/queries",
      ~decode=CodingQueryT.Table.t_decode,
      ~params={"viewingAs": viewingAs->UserT.Id.view},
    ),
  );
...

Basically this is how we do Safe-ish HTTP stuff safely hookin' into our Reductive / Bs-rx infra...

Notice each API.get call is required to pass a ~decode function.

As it stands currently if either of these decoders fail say because the id prop in the Record.t is the wrong type (which has happened to us in production at this point a few times now) currently we're Js.loging the Decco.decodeError record so we get this {path: "[0].id", message: "Not a int", value: nil} which doesn't point to this particular Record.t as we have a bunch of types with id properties.
So debugging this took me a good while to track down.

Preferably and what this PR add's to the project is that it forces callers of the Decco.typeFromJson functions to pass __LOC__ as an argument. Sort of forces callers into the happy path with the minimal amount of boilerplate.

Without these changes the callers would have to do something like ~decode=CodingQueryT.Table.t_decode >> Result.catchError(_ => Js.log("Failure at "++__LOC__), which is fine but it's not enforced and because it's extra work chances are people won't do it.

It also doesn't fix the location info for the messages pointing to failures for decco generated decoders like the id property on that Record.t type...

These changes result in an error with some location info baked in EG: {path: "[0].id File "CodingQuery.re", line 8, characters 5-20", message: "Not a int", value: nil} which tells the developer where exactly the failure occurred for a generated Decco decoder.

Which! Is why I'd generally prefer to bake it into the lib in a more transparent way but that's where I'd need your help @ryb73. Otherwise, I think these changes build in enough value as they are to solve for at least my teams use case and I hope they can be helpful to others using this as well.

[erlandsona]

Any thoughts on this @ryb73 ?

[davesnx]

This PR seems that it fixes #63 and it's really an improvement over the error messages that decco provides now. Can we do something to resolve this?

I understand the differences in opinions, but I tend do agree that having the location as a default for the lib would be very beneficial.

[TheSpyder]

This isn't something I'd want on by default. I use the decco errors to generate validation errors in my library API, and I wouldn't want my source file names leaking into those.

I can see it being useful in a standalone app scenario, but it's definitely not in a library scenario.

[ryb73]

I agree with @TheSpyder, this doesn't belong in a library. Moreover, unless I'm misunderstanding the changes in this PR, it's unrelated to issue #63. I'd be happy to accept a PR that does fix #63.