mlms13 / bs-decode

Type-safe JSON decoding for ReasonML and OCaml
https://mlms13.github.io/bs-decode/docs/what-and-why
MIT License
103 stars 18 forks source link

Nesting decoders is not obvious #55

Closed scull7 closed 4 years ago

scull7 commented 5 years ago

I had a lot of trouble determining how to nest decoders. I believe this is due to the pipe function not being exported from the Pipeline module.

The following code works but it is certainly not obvious this is how it should be done.

module Meta = {
  type t = {
    id: string,
    comment: string,
  };

  let make = (id, comment) => { id, comment };

  let fromJson = json =>
    Decode.AsResult.OfParseError.Pipeline.(
      succeed(make)
      |> at(["meta", "id"], string)
      |> at(["meta", "comment"], string)
      |> run(json)
    );
};

type t = {
  meta: Meta.t,
  example: string,
  other: string,
};

let make = (meta, example, other) => { meta, example, other };

let fromJson = json =>
  Decode.AsResult.OfParseError.Pipeline.(
    succeed(make)
    |> map2((|>), Meta.fromJson) // This is very cryptic, copy of the `pipe` function.
    |> field("example", string)
    |> field("other", string)
    |> run(json)
  );

Is there a better way to do this? Should the pipe function be exposed?

mlms13 commented 5 years ago

Is the shape of the JSON something like:

{
  "example": "this is text",
  "other": "other text",
  "meta": {
    "id": "abc123",
    "comment": "the meta comment"
  }
}

If so, then the decode function for Meta.t could look like:

let fromJson = json =>
  Decode.AsResult.OfParserError.Pipeline.(
    succeed(make)
    |> field("id", string)
    |> field("comment", string)
    |> run(json)
  );

And the main decoder could pass only the sub-object into the Meta decoder:

let fromJson = json =>
  Decode.AsResult.OfParseError.Pipeline.(
    succeed(make)
    |> field("meta", Meta.fromJson)
    |> field("example", string)
    |> field("other", string)
    |> run(json)
  );

However, I should definitely add some examples of this to the docs, and I'm not opposed to exposing pipe in the interface... I think that was just an oversight, and I've used it in cases when the JSON is structured more flatly than the Reason data model.

scull7 commented 5 years ago

Thank you for responding. If only the JSON was so well formatted. I'm working with xml2js output and it's not very friendly for the most part. Here is an example response:

{
  Response: {
    "$": {
      Directive: "Execute Query",
      ServerName: "Some Server",
      Version: "2.2",
    }
    Status: [ "OK" ],
    Timestamp: [ "09/16/2019 08:25:09" ],
    QueryResults: [
      {
        "$":  { count: "100" },
        Rows: [
          {
            "$": { number: "1" },
            ID: [ "1234567890" ],
            FIRSTNAME: [ "Some" ],
            LASTNAME: [ "Person" ],
            STREET: [ "123 Some St" ],
            CITY: [ "Somewhere" ],
            PHONE: [ "5551234567" ],
            EMAIL: [ "someone@somewhere.org" ],
            EMPTYFIELD: [ "" ],
            ALSOEMPTY: [ "NA" ],
            ANOTHERWAYITSEMPTY: [ "Unk." ],
          }
        ]
      }
    ]
  }
}

So there are zero or more rows, and each item in the rows may or may not have all the fields, which may or may not be empty in a myriad of ways. The Metadata is, unfortunately, sibling and cousin data to the query results. Thus, I couldn't use the field function. Perhaps you can show me a better way to parse this monster.

mlms13 commented 5 years ago

Ah, sorry I forgot about this and completely left you hanging. Did you have any luck? I just pushed a change (and I'll make a 0.9 release momentarily) that adds pipe to the public interface, which will definitely help with decoding siblings and cousins.

For filtering specific strings, could something like this work?

let v =
  Decode.(
    optionalField("FOO", list(string))
    |> map(Option.flatMap(List.head))
    |> map(Option.filter(List.containsBy(String.eq, _, ["", "NA", "Unk."])))
  );

Basically parse as an optional list of string, grab the first (if any), and turn any Some("NA") into None.

scull7 commented 5 years ago

Not at a place where I can test the code, but:

|> map (Option.filter(List.containsBy(String.eq, _, ["", "NA", "Unk."])))

Wouldn't that only keep items which contain any of ["", "NA", "Unk."]?

Did you mean?:

|> map(Option.filterNot(List.containsBy(String.eq, _, ["", "NA", "Unk."])))

Or am I getting the logic reversed?

scull7 commented 5 years ago

Thank you for taking the time to answer.

mlms13 commented 5 years ago

I think you're right. I was trying to think through whether it was filter or filterNot without actually testing it, and it looks like I picked wrong. :slightly_smiling_face: Booleans are hard.

0.9.0 is on npm now. Let me know if that makes your life any easier, otherwise I'm happy to keep looking for ways to improve this.

scull7 commented 5 years ago

Yes, only 2 possibilities and most of the times it seems they're all wrong. Thank you for the update on 0.9.0 I'll check it out this weekend and let you know.

mlms13 commented 4 years ago

I was hoping that applicative/monadic let syntax would be the perfect solution to this, but it's fairly clear now that that isn't the direction Reason is heading. Hopefully exposing the pipe function was enough to make it work for you!