Replace build_json! with a JSON builder API

[SabrinaJewson]

Description

Instead of:

build_json! {
    "a": b,
    optional "c": d,
}

we now use:

json_builder()
    .field("a", b)
    .optional_field("c", d)
    .build()

I would’ve used an array-based API like in #328 but that doesn’t really work well here because the field value types can be any type rather than just strings.

Dependencies

None.

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Please also list any relevant details for your test configuration

• Test A: cargo test --features env-file

[marioortizmanero]

Oh, nice! A builder pattern is a great solution, too. Thoughts:

• What about renaming field to required and field_optional to just optional? Easier to read, IMO.
• Why not use JsonBuilder::new instead of json_builder? That looks more like a regular builder pattern.
• What if we went for a similar approach for the map version? Just for "consistency", and we could possibly use some magic that avoids all these .map(|x| x.as_ref()) and .map(|x| x.as_deref()). Not quite sure how that could be done, though, just an idea. Might not be worth it in the end, but we could explore it.

[SabrinaJewson]

I only chose json_builder() because I think it’s prettier, but I have no real preference. I’ll open a new PR for replacing build_map with a MapBuilder (wait actually, we would lose the with_capacity, that’s not ideal…).

[marioortizmanero]

I mean, we're also losing with_capacity here (see the previous macro). Both PRs will affect performance either way. I guess it's just making a decision between performance and "usability", but that's quite subjective as well.

[SabrinaJewson]

Well, it would actually be possible to not affect performance. You just need to leverage type inference a bit:

pub(crate) struct JsonBuilder<Len> {
    map: serde_json::Map<String, serde_json::Value>,
    len: PhantomData<fn() -> Len>,
}

impl<Len: ToUsize> JsonBuilder<Len> {
    pub(crate) fn new() -> Self {
        Self {
            map: serde_json::Map::with_capacity(Len::to_usize()),
            len: PhantomData,
        }
    }
}

impl<Len: ToUsize> JsonBuilder<S<Len>> {
    pub(crate) fn required(mut self, name: &str, value: impl Serialize) -> JsonBuilder<Len> {
        self.map
            .insert(name.to_owned(), serde_json::to_value(value).unwrap());
        JsonBuilder {
            map: self.map,
            len: PhantomData,
        }
    }

    pub(crate) fn optional(self, name: &str, value: Option<impl Serialize>) -> JsonBuilder<Len> {
        if let Some(value) = value {
            self.required(name, value)
        } else {
            JsonBuilder {
                map: self.map,
                len: PhantomData,
            }
        }
    }
}

impl JsonBuilder<Z> {
    pub(crate) fn build(self) -> serde_json::Value {
        serde_json::Value::Object(self.map)
    }
}

pub(crate) struct Z;

pub(crate) struct S<T>(T);

pub(crate) trait ToUsize {
    fn to_usize() -> usize;
}

impl ToUsize for Z {
    fn to_usize() -> usize {
        0
    }
}

impl<T: ToUsize> ToUsize for S<T> {
    fn to_usize() -> usize {
        T::to_usize() + 1
    }
}

Do you think it's worth it?

[marioortizmanero]

Hah, awesome, very haskell-y. I was thinking of just using const generics but it's even better like that. Is the PhantomData<fn() -> Len> to make the type covariant over Len? But wouldn't PhantomData<Len> do the same?

I would say this is much better than the macro. Perhaps not as understandable if you aren't used to const generics, but macros are pretty weird, too. As long as it's nicely commented I would say it's great. I would also rename Z to Zero and S to Natural, since I've only seen that naming in Haskell (I assume you were inspired by it? Or just pure Rust?). And maybe ToUsize could be IncrUsize to emphasize on how it increments the constant whenever it's called?

Props for such a neat design!!

[SabrinaJewson]

Is the PhantomData<fn() -> Len> to make the type covariant over Len? But wouldn't PhantomData<Len> do the same?

I use PhantomData<fn() -> Len> out of principle, because it is usually the more desired type. To me, PhantomData<Len> sticks out in code as “something funny’s going on here” because usually you don’t want to inherit Send and Sync bounds from the inner type. But I could change it easily.

I assume you were inspired by it?

I was inspired by Peano arithmetic, in which there is a constant Z and a function S. But yeah, probably better to use more descriptive names. I’ll rename to Zero and Successor.

And maybe ToUsize could be IncrUsize to emphasize on how it increments the constant whenever it's called?

I think ToUsize is a good name, because conceptually a type like S<S<Z>> represents 2, so calling <S<S<Z>>>::to_usize() should yield 2.

---

Now I’ve actually implemented it, and I named ToUsize Natural as well as made the value an associated constant.

[marioortizmanero]

I use PhantomData<fn() -> Len> out of principle, because it is usually the more desired type. To me, PhantomData sticks out in code as “something funny’s going on here” because usually you don’t want to inherit Send and Sync bounds from the inner type. But I could change it easily.

No worries. It especially makes sense in this case; PhantomData<Len> would probably be better if we had a pointer to Len or something like that.

I would say that a comment explaining the fn() -> Len would be nice, but it might be that I just hadn't seen that myself before.

I was inspired by Peano arithmetic, in which there is a constant Z and a function S. But yeah, probably better to use more descriptive names. I’ll rename to Zero and Successor.
I think ToUsize is a good name, because conceptually a type like S<S> represents 2, so calling <S<S>>::to_usize() should yield 2.

Cool, I hadn't seen those in Rust yet. I think the naming is spot-on now. I also prefer it with associated constants.

[marioortizmanero]

Really nicely commented! It seems like this branch is over map-builder, so we would need MapBuilder implemented before merging this PR.

[SabrinaJewson]

Sorry about forgetting about this one! Merge conflicts are gone now.

[marioortizmanero]

No worries, thank you very much for the contribution! I'm merging this.